Skip to content
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

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Apply fix to prevent horizontal scrollbar in the CMS admin #1629

merged 1 commit into from
Jan 23, 2024

Conversation

lerni
Copy link
Contributor

@lerni lerni commented Dec 1, 2023

As mentioned in #1430 (comment) to make the suggested fix work with current 5.x 7b430d4 is manually reverted in this PR.

@GuySartorelli
Copy link
Member

GuySartorelli commented Dec 1, 2023

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?

@michalkleiner michalkleiner changed the title Fixes https://github.com/silverstripe/silverstripe-admin/issues/1430 - prevents horizontal scrollbar Apply fix to prevent horizontal scrollbar in the CMS admin Dec 3, 2023
@GuySartorelli
Copy link
Member

(fyi I've updated the PR description and your latest comment to refer to the commit hash without a #, so that GitHub can link to it)

Copy link
Member

@GuySartorelli GuySartorelli left a 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.

@lerni
Copy link
Contributor Author

lerni commented Dec 18, 2023

OK finally I know what https://github.com/silverstripeltd/product-issues/issues/630 was about (private repo) and now can reproduce.

fluen-choosen-fix.mp4

Prepared a workaround to prevent regression, but it needs some more testing.

@GuySartorelli
Copy link
Member

@lerni How's it going with this? Have you had a chance to test your workaround yet?
Feel free to include the workaround here if you like and I can see what does/doesn't work.

@lerni
Copy link
Contributor Author

lerni commented Jan 19, 2024

Oh my, rebase went wrong - 'll try some more git foo. Basically now .chosen-drop (element inside container) has overflow: hidden if active.

.chosen-container:not(.chosen-container-active) .chosen-drop {...}

@lerni lerni changed the base branch from 2 to 2.1 January 21, 2024 09:50
@lerni lerni requested a review from GuySartorelli January 21, 2024 21:18
refines #7b430d4 hide elements inside chosen-container if not active
@GuySartorelli
Copy link
Member

Force pushed (with no code change) to get CI to run.
I took that opportunity to update the commit message to include the FIX prefix as well.

Copy link
Member

@GuySartorelli GuySartorelli left a 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.

@GuySartorelli GuySartorelli merged commit c8f03f2 into silverstripe:2.1 Jan 23, 2024
12 checks passed
@GuySartorelli
Copy link
Member

Thanks for taking the time to get this over the line 💙

@lerni lerni deleted the horizontal-scroll branch January 24, 2024 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants