Followup from #3177238: Stronger visual affordance for Olivero's textbox form elements (Design only).

We need to implement the new form designs for Olivero. Note that this issue does not include implementing new designs for fieldsets (which is handled in #3194468: Refine designs for Olivero details and fieldsets).

Figma designs: https://www.figma.com/file/mqpBzVACACfsvwOw1C0ZBJ/Olivero-Forms-Explorat...

Comments

mherchel created an issue. See original summary.

mherchel’s picture

Assigned: Unassigned » mherchel
Status: Active » Needs work
StatusFileSize
new48.99 KB

Some initial work on this. Still not done.

mherchel’s picture

Issue summary: View changes
mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new113.87 KB

Patch attached. This needs review!

A tugboat preview can be found at https://3194350-forms-2-uxdbja8p1vj23ttknnwraqkvaevzpbq2.tugboat.qa/
Note that there are webform and style guide example forms.

mherchel’s picture

StatusFileSize
new113.48 KB

Updated patch fixes the autocomplete icon.

mherchel’s picture

Videos attached. Note that videos can't quite show everything, so please look around the Tugboat preview at https://3194350-forms-2-uxdbja8p1vj23ttknnwraqkvaevzpbq2.tugboat.qa/

mherchel’s picture

StatusFileSize
new113.61 KB
new2.68 KB

Found an issue on Chrome on Android where the browser was defaulting to a gray background for select and input[type="date"] elements.

Fix attached.

oksana-c’s picture

StatusFileSize
new72.14 KB
new72.11 KB

the new form design looks slick! and so easy to preview with Tugboat.

I noticed that the number field has width set to 4em and if the max for field is a two or three digit number, then the second and third digit is cut off. Adding screenshots. It'd be best to keep the width of the field at `auto`.

number field with set width

number field with set width

number field with auto width

number field with auto width

I haven't dug through any source code and not certain if the 4em value comes from Olivero, so may not be in scope here.

mherchel’s picture

Good catch. That is out of scope, because those styles are being added by the webform module. Thanks for the review!

mherchel’s picture

StatusFileSize
new114.68 KB

This latest patch removes some old CSS variables that were not transpiling (because they didn't exist).

mherchel’s picture

Gave @andrewmacpherson a walk though on this yesterday. He didn't see any major issues, although it definitely needs more thorough testing.

andrewmacpherson’s picture

Status: Needs review » Reviewed & tested by the community

Sorry for being slow to RTBC this one. Confirming comment #11 - @mherchel and I did walk through the implementation during a video call.

I didn't see any significant deviations from the agreed designs from the earlier issue, so the implementation (broadly) looks good to go. We did note that in Windows high-contrast mode, a few things could do with some extra polish for the sake of elegance, but I don't think we saw any blockers there.

So I recommend we commit the work done so far, and if any glitches or accessibility issues are noticed afterwards they can have more specific issues. What we have here is several big steps forward.

mherchel’s picture

StatusFileSize
new116.03 KB
new5.34 KB

Yay!

Some minor code cleanup changes in this patch.

mherchel’s picture

StatusFileSize
new116.55 KB
new10.8 KB

Forgot to transpile the CSS 🤪

New patch and interdiff attached.

alexpott credited lauriii.

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new14.65 KB
new3.33 KB

I reviewed this with @lauriii - we took it for a test drive and it always lovely to see a see diff stat 31 files changed, 326 insertions, 1584 deletions. yum.

The focus state of inputs does quite match the design. There are rounded corners on the design but with the patch there aren't and the elements also get unrounded corners so it looks a bit odd.

Focus state on input element

Also we couldn't decide if the check in a checkbox was correctly aligned - not sure that it is as central as it should be:

lauriii’s picture

Issue summary: View changes
StatusFileSize
new57.67 KB

I noticed that the dropbutton elements can be hard to distinguish from each other when the tray is open because it doesn't have any border. See this gif for an example of that:

mherchel’s picture

Good catch on the drop buttons!

The focus state of inputs does quite match the design

I reviewed the focus designs with @jwitkowski79, and she signed off on the focus state issue. The gist of the matter is that outlines do not support rounded corners (unless they're set to auto, but then you cannot change their color in FF & Safari). To replicate the rounded corners using borders and box shadows can cause layout shifts (which outlines do not do), and/or be invisible in Windows High Contrast mode (box shadows).

I'll get her to comment in here. If you don't like them, we can create a followup issue.

alexpott’s picture

@mherchel re focus styles - if both you and @jwitkowski79 are happy I'm happy! I had to comment because all I had to go on were the designs in the issue summary and figma and the patch itself. If there are other areas where the implementation and the design don't match it'd be great to detail them.

jwitkowski79’s picture

Hello! Mike did indeed review the focus states with me and we discussed different options. Removing the rounded corners was the best way forward. I give my thumbs up!

mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new116.37 KB
new3.98 KB
new40.69 KB

Updated patch attached adding borders to the dropbutton.

Notes:

  • I added an outside border to the .dropbutton-widget container along with border-radius.
  • Because the widget now has border radius, I was able to remove the border-radius values (including a lot of special RTL code)
  • I also added separator borders for between each item.

Image:

ressa’s picture

Thanks @mherchel, I tested the new patch with latest Drupal 9.2.0-dev in Firefox, Chromium, and GNOME Web browsers (OS: Ubuntu) and it looks great, and just as in your dropbutton.gif.

alexpott’s picture

StatusFileSize
new233.28 KB

It doesn't look so great on the content types page see below...

Also the way the light blue bleeds over the drop button separator looks odd.

mherchel’s picture

yeah, tbh, we could completely re-write those drop buttons to be more in-line with what Claro has done.

Would you be okay committing this and creating a stable-blocking followup issue on that? I feel that I want to get this in sooner than later so to make it easier to do additional testing.

mherchel’s picture

I asked @alexpott and @lauriii in Slack if we can break out the drop-buttons into another issue and they gave the 👍

Opened up #3200370: Fix Olivero's drop-button style to conform with new form styles

mherchel’s picture

StatusFileSize
new118.38 KB
new3.33 KB
new25.49 KB

Updated patch with minor change that resolves the checkbox centering issue that @alexpott mentioned in #16.

The change in the styling is below in this animated gif

mherchel’s picture

katannshaw’s picture

Status: Needs review » Reviewed & tested by the community

Just reviewed patch #26. LGTM. Marking RTBC.

mherchel’s picture

mherchel’s picture

mherchel’s picture

mherchel’s picture

mherchel’s picture

lauriii’s picture

I reviewed the code and did some cross-browser compatibility testing with @mherchel. @mherchel has opened some follow-ups for most of the issues discovered so we could land this issue as soon as soon as possible.

+++ b/core/themes/olivero/css/components/form-select.pcss.css
@@ -6,187 +6,81 @@
+  border-radius: 3px;

I did find one nitpick on the code. This should probably use --border-radius variable.

mherchel’s picture

StatusFileSize
new617 bytes
new118.39 KB

Resolving #34

Note that I did compile the CSS with this patch, but there is no change within the compiled CSS.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: 3194350-35.patch, failed testing. View results

mherchel’s picture

Status: Needs work » Reviewed & tested by the community

Setting back to RTBC. The last test failure was unrelated, and the newest test pass passed.

lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed 03bf48d and pushed to 9.2.x. Thanks!

  • lauriii committed 03bf48d on 9.2.x
    Issue #3194350 by mherchel, alexpott, oksana-c, lauriii, jwitkowski79,...

Status: Fixed » Closed (fixed)

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

gábor hojtsy’s picture