- ๐ฎ๐ชIreland markconroy
I'm going to bump this to major. What we are seeing in the screenshots is not acceptable for a demo of how great Drupal is. And I think a fix such as "let's just put a white background colour behind the preview info" would be a good start to have it acceptable. After that, maybe we could look at a fresh design for this component.
- ๐ซ๐ฎFinland lauriii Finland
+1 that we could approach this with a quick fix first!
- Status changed to Needs work
almost 2 years ago 4:17pm 17 February 2023 - ๐ฎ๐ชIreland markconroy
Looks like this is an issue with Drupal core generally, not just with Umami.
In node.preview.css we have:
.node-preview-container { position: fixed; z-index: 499; box-sizing: border-box; width: 100%; padding: 10px; }
We can remove all of that, and just leave in the
padding: 10px
to fix this issue.Here's some screenshots:
Umami currently
Umami if we remove the position: fixed CSS
Olivero currently
Olivero if we remove the position: fixed CSS
In the mean time, let's remove this from the Umami issue queue and move to to the theme system.
- @markconroy opened merge request.
- Status changed to Needs review
almost 2 years ago 4:49pm 17 February 2023 - ๐ซ๐ฎFinland lauriii Finland
I'm not sure we should be making this change this broadly. It seems like this could break some pre-existing themes. Even if it wouldn't, it would be changing the behavior in those themes. If we were to introduce that kind of change, we'd probably want to evaluate that a bit more. I think it would be easier to land a solution that fixes this in the scope of Umami. โบ๏ธ
- ๐ฉ๐ชGermany rkoller Nรผrnberg, Germany
i've applied the patch to a fresh 10.1.x-dev install with the demo_umami profile. the change looks straight forward and good. the only small detail i consider at least distracting on wider viewports is the position of the view mode switcher. it is shifted to the left compared to the position of the search field which is more or less in the center of the browser window. i've intentionally dragged the browser window to the maximal possible width to illustrate the offset/shift (screenshot is taken in safari 16.1 - but tested also in the latest version of firefox).
one other detail i've noticed which is definitely out of the scope of the issue probably. but the tabbing order is sort of unexpected. if you start tabbing from the
start to content editing
link you get next to the select list, then theswitch
button. but me as a user would expect to get to the language selection (english/espanol) next but instead i tab into the admin toolbar. would i make sense to move the node preview container div down the DOM after the the toolbar-administration div? - ๐ฎ๐ชIreland markconroy
Thanks Lauriii. I'll move this back to Umami, get a small fix for it there, and see how we progress then for Drupal core.
- ๐ฎ๐ชIreland markconroy
@lauriii this is ready for a review again.
- ๐ซ๐ฎFinland lauriii Finland
Discovered another issue which we should fix for optimal preview UX: ๐ 'node' variable in page.html.twig is not available on preview node page Fixed .
I pushed some changes to the MR to make the preview bar slightly nicer.
The "switch" button should also not be visible when JavaScript is enabled. The button is visible because there's some CSS in Umami that overrides the
.js .js-hide
styles. I added!important
to thedisplay: none
to avoid other CSS accidentally overriding that. We could also fix this in Umami but it feels a bit silly. That change probably shouldn't go to stable. - ๐ฎ๐ชIreland markconroy
Looks good to me. Will we RTBC it?
And perhaps create a follow up to see why we need to use !important. Umami should not be overriding .js-hide.
- ๐ซ๐ฎFinland lauriii Finland
If the changes look good for you, go for it!
I think we may want to keep the
!important
there because it helps prevent accidental overrides. - Status changed to Needs work
almost 2 years ago 6:50pm 18 February 2023 - ๐บ๐ธUnited States smustgrave
When I apply the MR changes and this is what I see. Is it okay for the view mode to overlap the search field?
- ๐ซ๐ฎFinland lauriii Finland
@smustgrave did you clear caches after applying the patch?
- Status changed to RTBC
almost 2 years ago 7:02pm 18 February 2023 - ๐บ๐ธUnited States smustgrave
Maybe weekend work wasn't my friend after all. Thought all the changes took affect.
Clearing cache fixed it.
- Status changed to Needs work
over 1 year ago 6:53am 29 March 2023 - ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
Agree, this is much better, manually tested before and after - looks good to me.
I committed this locally and was about to push when the !important in system module gave me pause.
I know it *should* be hidden, but its possible this will cause an unexpected regression for someone who (wrongly) had put some other specificity on a component that has js-hide to change the display.
E.g I could envision a scenario where someone did this
<div class="js-hide" id="something"></div>
With css
#something { display: block }
But with this change their previous ID based selector would no longer win.
So can we do this without changing system js.module.css?
See https://codepen.io/larowlan/pen/zYJXZVj for a pen of the above, put the !important in and see the behaviour change.
- Status changed to Needs review
over 1 year ago 2:47pm 29 March 2023 - ๐ฎ๐ชIreland markconroy
@lauriii @larowlan I've removed that
!important
. I don't think it is needed as demo_umami profile and umami (theme) does not have any override that I can see for.js .js-hide
. I can't find the string.js
or.js-hide
anywhere inside the demo_umami directory.Here's 2 screenshots, showing the "Switch" not visible when JS is enabled and then showing it visible when JS is disabled.
- Status changed to RTBC
over 1 year ago 7:41pm 1 April 2023 -
lauriii โ
committed cc24ab06 on 10.1.x
Issue #3002443 by markconroy, lauriii, smustgrave, larowlan: Improve...
-
lauriii โ
committed cc24ab06 on 10.1.x
-
lauriii โ
committed 27e75830 on 10.0.x
Issue #3002443 by markconroy, lauriii, smustgrave, larowlan: Improve...
-
lauriii โ
committed 27e75830 on 10.0.x
-
lauriii โ
committed 79423dc9 on 9.5.x
Issue #3002443 by markconroy, lauriii, smustgrave, larowlan: Improve...
-
lauriii โ
committed 79423dc9 on 9.5.x
- Status changed to Fixed
over 1 year ago 7:14am 3 April 2023 Automatically closed - issue fixed for 2 weeks with no activity.