-
Notifications
You must be signed in to change notification settings - Fork 96
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
Apply fix to prevent horizontal scrollbar in the CMS admin #1629
Conversation
You mention that this undoes a commit which was part of #1422 - can you please validate whether the issue that pr fixed is regressing as a result of this change? There's also more to this change than undoing the commit you mentioned. Are the additional changes also required? |
(fyi I've updated the PR description and your latest comment to refer to the commit hash without a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes a regression - https://github.com/silverstripeltd/product-issues/issues/630
Note that the issue isn't talking about the locale dropdown in the left-hand vertical menu, it's talking about the locale dropdown when creating a new locale with fluent.
Can you please see if there's a way to resolve the scrolling bug without regressing the fluent locale bug?
Also, and sorry I didn't notice this earlier, but this PR will need to target the 2.1
branch so that we can release it as a patch.
OK finally I know what https://github.com/silverstripeltd/product-issues/issues/630 was about (private repo) and now can reproduce. fluen-choosen-fix.mp4Prepared a workaround to prevent regression, but it needs some more testing. |
@lerni How's it going with this? Have you had a chance to test your workaround yet? |
Oh my, rebase went wrong - 'll try some more git foo. Basically now .chosen-container:not(.chosen-container-active) .chosen-drop {...} |
refines #7b430d4 hide elements inside chosen-container if not active
Force pushed (with no code change) to get CI to run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - if CI goes green I'll merge.
Thanks for taking the time to get this over the line 💙 |
As mentioned in #1430 (comment) to make the suggested fix work with current 5.x 7b430d4 is manually reverted in this PR.