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

Fix sticky admin nav on long menu #4884

Conversation

MassimilianoLattanzio
Copy link

Summary

In Solidus, the admin nav menu becomes sticky after scrolling the page. If the menu contains a few entries, everything works fine. But if the menu includes some entries and the height exceed the window height, since the menu is sticky, the latest entries will be hidden.

Example with short menu:

Screen.Recording.2023-01-26.at.18.12.45.mov

Example with long menu:

Screen.Recording.2023-01-26.at.18.14.59.mov

This PR applies the sticky only if the menu height does not exceed the window height.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • I have used clear, explanatory commit messages.
  • I have documented new code with YARD.

The following are not always needed (cross them out if they are not):

  • I have added automated tests to cover my changes.
  • I have attached screenshots to demo visual changes.
  • I have opened a PR to update the guides.
  • I have updated the README to account for my changes.

In Solidus, the admin nav menu becomes sticky after scrolling the page.
If the menu contains a few entries, everything works fine.
But if the menu includes some entries and the height exceed the window
height, since the menu is sticky, the latest entries will be hidden.

This commit applies the sticky only if the menu height does not exceed
the window height.
@MassimilianoLattanzio MassimilianoLattanzio requested a review from a team as a code owner January 26, 2023 17:22
@github-actions github-actions bot added the changelog:solidus_backend Changes to the solidus_backend gem label Jan 26, 2023
@MassimilianoLattanzio
Copy link
Author

This is the result after the fix:

Screen.Recording.2023-01-26.at.18.24.28.mov

@MassimilianoLattanzio
Copy link
Author

There are other CSS issues (the menu border and the white background between the primary and user menus). Solving these could be a bit tricky, but if this PR is accepted, I can work on them in a separate PR.

Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Many thanks 🙌

@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #4884 (79ab245) into master (1c4b1ff) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4884   +/-   ##
=======================================
  Coverage   86.25%   86.25%           
=======================================
  Files         577      577           
  Lines       14678    14678           
=======================================
  Hits        12660    12660           
  Misses       2018     2018           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks Massi!

@waiting-for-dev waiting-for-dev merged commit a1e3d38 into solidusio:master Jan 27, 2023
@MassimilianoLattanzio MassimilianoLattanzio deleted the ml/fix-sticky-admin-nav-on-long-menu branch January 30, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants