-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[UX] Fix outside-right
styles so text doesn't overlap
#6687
Comments
It's a definite improvement. |
I like the change. It is immediately clear that there are sub-menus and they are legible. I like the box shadow effect and think it helps make it stand out. I'm interested to hear what others think. |
This could be a good candidate for the next release. |
This is what we have now - to directly compare with the change: And that works like a charm. 👍 What I find a bit irritating is, that for RTL languages, the box shadow always appears, even if far from the window edge (and no overlap). What's the intention behind that? ps: this isn't a real RTL language, of course, I just changed the direction. 😉 |
@wesruv - This is an improvement, but I'm not sure if I would have noticed the improvement if I weren't looking for it. To my eyes it was not that visible. Could we do something a bit more dramatic? |
Nice catch @indigoxela. I think the newly added RTL CSS might have been added to the wrong selector. I posted a suggestion here: https://github.com/backdrop/backdrop/pull/4847/files#r1739451230 I think this is a nice little improvement. Also, I think it's a UX improvement and could be made post feature-freeze. |
outside-right
styles so text doesn't overlapoutside-right
styles so text doesn't overlap
The PR seems to only have box-shadow changes. I'm not seeing the change in background color, as in @wesruv 's first screenshot: I see the darker black-gray is still there: Did we decide against the background color? It's possible that could warrent a larger discussion, since right now every item with a child gets a distinct color, and the suggestion above would be using that color to indicate soemthing other than "has a child link". I'm not sure the current pattern is a good pattern, but that's for another issue :) |
I think we have some other work to do on the colors, so perhaps saving that for later is good. Here's what other menu links look like with the box-border changes only: And the original Date formats: Edit: I have opened a follow-up issue for the background colors: #6710 |
I've also re-applied the RTL changes as @quicksketch recommended, and here's what that looks like now: |
Confirmed @jenlampton's changes work as expected. The additional dropshadow and border are added only after the menu begins to wrap, both in LTR and RTL languages. |
By @wesruv, @jenlampton, @izmeez, @indigoxela, and @quicksketch. Co-authored-by: Wes Ruvalcaba <[email protected]>
I merged backdrop/backdrop#4869 into 1.x for 1.29.0. Thank you @wesruv for creating the issue and filing the initial PR! backdrop/backdrop@5aa692b by @wesruv, @jenlampton, @izmeez, @indigoxela, and @quicksketch. |
As reported by @izmeez and further tracked down by @avpaderno, at tablet-ish screen sizes the admin menu fly outs too close to the right side of the screen get text overlap.
I found the issue is with the
outside-right
styling, will be sending a PR shortlySteps To Reproduce
To reproduce the behavior:
The text was updated successfully, but these errors were encountered: