Skip to content

Conversation

Charlie-XIAO
Copy link
Contributor

See the screenshot:

The dropdown menu is cut off because it is aligned to the left boundary with left: 0. With this PR it will be aligned to the right boundary with right: 0.

Copy link

github-actions bot commented Jan 4, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 07ff721. Link to the linter CI: here

Copy link
Member

@virchan virchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn’t reproduce the cut-off shown in the screenshot, after viewing the documentation in Firefox, Microsoft Edge, and Chrome:

Firefox (170% Zoom)
Current PR
Microsoft Edge (175% Zoom)
Current PR
Chrome (175% Zoom)
Current PR

However, I did notice that the PR’s implementation shifts the navigation bar from horizontal to vertical, as observed in Edge and Chrome.

Perhaps I’m missing an important detail here?

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Jan 5, 2025

@virchan Thanks for the review - I should have made things clearer: You need to choose a non stable or dev version because their labels are shorter than the stable and dev versions, in which case the dropdown menu will expand out of the screen.

In other words this PR will make a noticeable difference only after 1.7.X becomes a non stable or dev version, unless backported to 1.5.X and 1.6.X as well.

Copy link
Member

@virchan virchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification, @Charlie-XIAO! It's really helpful! I can see the cut-off when viewing the document online in version 1.5.2.

Thank for fixing it as well!

@virchan virchan added the Waiting for Second Reviewer First reviewer is done, need a second one! label Jan 5, 2025
.version-switcher__menu {
// The version switcher is aligned right so we need to avoid the dropdown menu
// to be cut off by the right boundary
left: unset;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is left: unset required here?

Suggested change
left: unset;

In chrome, I see that it gets set to left: 0 anyways:

SCR-20250105-jnng

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomasjpfan Thanks for pointing out - the unset is needed, but indeed the selector was not strong enough. 07ff721 hopefully fixes it.

And just for reference, this is the desired effect:

not this (because long labels will not have enough space in the dropdown if selecting a short label):

or this (the original, because the long labels will go out of the right boundary of the screen if selecting a short label):

@thomasjpfan thomasjpfan merged commit a7bce39 into scikit-learn:main Jan 5, 2025
30 checks passed
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jan 8, 2025
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jan 8, 2025
@Charlie-XIAO Charlie-XIAO deleted the doc/version-switcher-align branch February 16, 2025 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants