Broken out from issue #3194350: Implement new form element designs for Olivero. We need some additional work on the drop-buttons.

Ideally, I'd like to copy what Claro has done (and refactor it as needed).

We also need to ensure that the drop-button's various focus styles are highly visible (they currently are not)

CommentFileSizeAuthor
#42 interdiff-40-42.txt1017 bytesmherchel
#42 3200370-42.patch12.25 KBmherchel
#41 3200370-single-dropdown.png9.19 KBindrajithkb
#40 interdiff-37-40.txt982 bytesmherchel
#40 3200370-40.patch12.21 KBmherchel
#38 3200370-with-limited-width.png38.94 KBindrajithkb
#38 3200370-after-patch.png54.14 KBindrajithkb
#38 3200370-before-patch-long-word.png36.73 KBindrajithkb
#38 3200370-single-item.png7.68 KBindrajithkb
#37 dropbutton.gif244.67 KBmherchel
#37 interdiff-35-27.txt1.34 KBmherchel
#37 3200370-37.patch12.19 KBmherchel
#36 Screen Shot 2021-05-10 at 13.04.48.png11.86 KBlauriii
#36 Screen Shot 2021-05-10 at 13.01.59.png14.26 KBlauriii
#35 interdiff-27-35.txt1.06 KBmherchel
#35 3200370-35.patch12.15 KBmherchel
#29 AP 3200370.png315.75 KBchetanbharambe
#29 BP 3200370.png296.54 KBchetanbharambe
#27 interdiff-19-27.txt3.27 KBmherchel
#27 3200370-27.patch12.15 KBmherchel
#25 interdiff.txt501 byteskapilv
#25 3200370-25.patch11.86 KBkapilv
#23 Screenshot 2021-04-20 at 14.00.31.png89.6 KBalexpott
#21 3200370-21-button-offset-fixed.png10.75 KBressa
#20 after-patch.mp41.17 MBbhumikavarshney
#19 3200370-19.patch11.87 KBmherchel
#19 interdiff-15-19.txt1.35 KBmherchel
#19 interdiff-15-19.txt1.35 KBmherchel
#18 Screen Shot 2021-04-08 at 15.43.02.png2.14 KBlauriii
#17 3200370-17.png45.22 KBkleiton_rodrigues
#15 3200370-15.patch12 KBmherchel
#15 interdiff-11-15.txt2.63 KBmherchel
#11 interdiff-10-11.txt3.86 KBmherchel
#11 3200370-11.patch11.88 KBmherchel
#10 interdiff-10-9.txt435 bytesimalabya
#10 3200370-10.patch22.44 KBimalabya
#9 Screenshot 2021-03-04 at 2.52.59 AM.png47.85 KBimalabya
#9 interdiff-9-4.txt1.71 KBimalabya
#9 3200370-9.patch22.44 KBimalabya
#8 single-dropbutton.png167.72 KBmherchel
#6 3200370-after.gif4.23 MBabhijith s
#6 3200370-before.gif3.33 MBabhijith s
#5 Screenshot 2021-03-03 at 10.21.24.png46.39 KBgauravvvv
#4 3200370-dropbutton.mp46.53 MBmherchel
#4 3200370.patch10.63 KBmherchel
#4 dropbuttons.png48.57 KBmherchel

Comments

mherchel created an issue. See original summary.

mherchel’s picture

Title: Fix drop-button style to conform with new form styles » Fix Olivero's drop-button style to conform with new form styles
mherchel’s picture

Issue summary: View changes
mherchel’s picture

Status: Active » Needs review
StatusFileSize
new48.57 KB
new10.63 KB
new6.53 MB

Patch attached.

Here's what it looks like. I also attached a movie showing multiple browsers and high contrast mode.

gauravvvv’s picture

StatusFileSize
new46.39 KB

#4, Looks fine to me. RTBC +1

abhijith s’s picture

StatusFileSize
new3.33 MB
new4.23 MB

Applied patch #4 and it works fine.

Before patch:
before

After patch:
after

RTBC +1

mherchel’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the review! Don't forget to change the status to RTBC when you RTBC issues :D

mherchel’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new167.72 KB

Found a bug in this. It doesn't account for the single variant of the dropbutton.

imalabya’s picture

Status: Needs work » Needs review
StatusFileSize
new22.44 KB
new1.71 KB
new47.85 KB

@mherchel Please review the patch which addresses the single variant. Attached screenshots.

imalabya’s picture

StatusFileSize
new22.44 KB
new435 bytes

Updated patch

mherchel’s picture

StatusFileSize
new11.88 KB
new3.86 KB

It looks like you added the patch to the patch :D (patchception!)

I'm attaching an updated patch. I removed the width: auto, and also added some RTL fixes.

katannshaw’s picture

Status: Needs review » Reviewed & tested by the community

Looks and works great. Marking RTBC!

mherchel’s picture

mherchel’s picture

Status: Reviewed & tested by the community » Needs work

Talking with @lauriii in slack and he had a comment on the z-indexes, but didn't get a chance to post it. We need to make sure we're following https://www.drupal.org/docs/theming-drupal/z-indexes-in-drupal-8

mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new2.63 KB
new12 KB

Updating patch by setting the z-index for open dropbuttons to 50 per Drupal's z-index documentation.

I also fixed an IE11 issue where the dropbuttons were expanding to full width when outside of tables.

proeung’s picture

Status: Needs review » Reviewed & tested by the community

Patch #15 look good and works great. Marking this RTBC!

kleiton_rodrigues’s picture

StatusFileSize
new45.22 KB

Works good! Moving to RTBC +1!

lauriii’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new2.14 KB

Few things I noticed while testing this:


  1. The text on the button appears to be off-center.
  2. The focus effect is inconsistent between the actions and the toggle button. On the actions the outline is rendered inside the button but on the toggle it's rendered outside.
mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new1.35 KB
new1.35 KB
new11.87 KB

The text on the button appears to be off-center.

Fixed this by removing the line-height.

The focus effect is inconsistent between the actions and the toggle button

Fixed with an outline-offset.

bhumikavarshney’s picture

StatusFileSize
new1.17 MB

Hi @mherchel
#19 patch Works good for me! Moving to RTBC +1!

ressa’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new10.75 KB

The button text is now aligned, and it looks great:

Button off-center fixed

mherchel’s picture

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new89.6 KB

I think there is a grey line appearing under the dropbutton in the down state that is not meant to be there. See:
Odd grey line

alexpott’s picture

This extra line is cause by the border: solid 1px transparent; in

.dropbutton-action:first-child {
    border: solid 1px transparent;
    border-right: solid 2px #fff; /* LTR */
    border-radius: 0.1875rem 0 0 0.1875rem; /* LTR */
  }

without that line the drop buttom looks way sharper.

kapilv’s picture

Status: Needs work » Needs review
StatusFileSize
new11.86 KB
new501 bytes

Address #24.

mherchel’s picture

Status: Needs review » Needs work

@KapilV Thanks for the patch, but you'll need to 1) ensure that you're working on the pcss.css version of the file and then compile the CSS version. You'll also want to find out why the transparent border was added and account for that use case.

I'm working on this right now.

mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new12.15 KB
new3.27 KB

We can't get rid of the transparent border because it's needed by Windows High Contrast users. That being said, I was able to fix the underlying issue by moving the background color from the container to the list item.

patch and interdiff attached.

chetanbharambe’s picture

Assigned: Unassigned » chetanbharambe
chetanbharambe’s picture

Assigned: chetanbharambe » Unassigned
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new296.54 KB
new315.75 KB

Verified and tested patch #27.
Patch applied successfully and looks good to me.

Testing Steps:
# Set the Olivero theme from Appearance.
# Goto: /admin/structure/types/manage/article/fields
# User should see dropdown-button style to conform with new form styles.

Looks good to me.
Can be a move to RTBC
Please refer attached screenshots for Before and After patch.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: 3200370-27.patch, failed testing. View results

catch’s picture

Status: Needs work » Reviewed & tested by the community

Restoring status after HEAD was broken.

mherchel’s picture

Created followup to fix text vertical alignment at #3212828: Olivero: Text within drop-button not vertically aligned

lauriii’s picture

+++ b/core/themes/olivero/css/components/dropbutton.pcss.css
@@ -1,76 +1,155 @@
+  z-index: 50; /* Ensure this appears above all other dropbuttons. */

Based on https://www.drupal.org/docs/theming-drupal/z-indexes-in-drupal-8, shouldn't this be 100?

mherchel’s picture

StatusFileSize
new12.15 KB
new1.06 KB

You are correct! Updated patch attached. Leaving RTBC as this is a very minor change.

lauriii’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new14.26 KB
new11.86 KB

  1. Long texts are not legible in browsers that don't support max-content

  2. Dropbuttons with just a single item are missing border radius on the right
mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new12.19 KB
new1.34 KB
new244.67 KB

Fixed the issue from #36. Note to test this, you must restrict the width of the parent .dropbutton-wrapper, and then edit the text of the action so it's very long.

indrajithkb’s picture

Status: Needs review » Needs work
StatusFileSize
new7.68 KB
new36.73 KB
new54.14 KB
new38.94 KB

Hi @mherchel the second point is still not fixed from #36 by @lauriii

1. Still we missed the border-radius
Attaching SS:
image

2. When test is long
SS after patch for long text (without limitting the wrapper width).
image

3. Here we couldn't see the separation between the two dropdowns
SS after patch for long text (with limiting the wrapper width).
image

All these text is edited through browser dev-tools.

Attaching the SS of long dropdown item before patch (might be helpful for finding the 3rd point):

image

mherchel’s picture

Good catch on the border radius. I completely missed that comment.

I'm not too worried about your point in #3. I'm not sure where the drop-button is going to be width constrained, and even if so, I'm not quite sure how we'd get to the point where the text gets long enough for it to wrap. We're getting into some very edge cases here, and if it does become a problem, I'd rather open up a followup issue.

I appreciate the thorough review, though!

mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new12.21 KB
new982 bytes

Here is a patch that adds back in the border-radius when it has a single action.

indrajithkb’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new9.19 KB

Hi @mherchel thanks for the patch, now it's working as expected.

Attaching screenshot after patch #40
image

This was remaining task on this issue , now it's get resolved. So am moving the issue to RTBC.

mherchel’s picture

StatusFileSize
new12.25 KB
new1017 bytes

One minor change: Adding cursor: pointer onto the button element.

lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed 61ebd08 and pushed to 9.3.x. Thanks!

  • lauriii committed 61ebd08 on 9.3.x
    Issue #3200370 by mherchel, imalabya, KapilV, Indrajith KB, lauriii,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.