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

Flyout menu: Render menu bottom-up if it exceeds the viewport height #5037

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

sukhwinder33445
Copy link
Contributor

fixes #4594

@sukhwinder33445 sukhwinder33445 added bug Something isn't working area/javascript Affects the javascript framework labels Jun 12, 2023
@sukhwinder33445 sukhwinder33445 self-assigned this Jun 12, 2023
@cla-bot cla-bot bot added the cla/signed label Jun 12, 2023
@sukhwinder33445 sukhwinder33445 changed the title Flyout menur: Render menu bottom-up if it exceeds the viewport height Flyout menu: Render menu bottom-up if it exceeds the viewport height Jun 12, 2023
@sukhwinder33445 sukhwinder33445 force-pushed the fix/menu-flyout-arrow-position-4594 branch 2 times, most recently from 99f6765 to a18d03a Compare June 12, 2023 11:54
@sukhwinder33445 sukhwinder33445 force-pushed the fix/menu-flyout-arrow-position-4594 branch from a18d03a to cebfb8d Compare June 19, 2023 11:12
public/js/icinga/behavior/navigation.js Outdated Show resolved Hide resolved
@sukhwinder33445 sukhwinder33445 force-pushed the fix/menu-flyout-arrow-position-4594 branch from cebfb8d to 4c965bc Compare August 8, 2024 14:17
Copy link
Contributor

@flourish86 flourish86 left a comment

Choose a reason for hiding this comment

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

Basically it works.

I found a problem, though, which can best be seen on this screenshot:
in extreme conditions this could fire back and now exceed the top
Screenshot 2024-08-08 at 16 44 59

Since the items outside of the screen aren't accessible anymore in this case, this should be prevented by restricting the height and make the flyout scrollable.

I made a quick mockup for this.

Frame 1

While the screenshot shows the collapsed version, this problem also occurs in the expanded version of the sidebar.

Screenshot 2024-08-08 at 16 55 56

Another thing, that I discovered, was that the padding doesn't adapt (properly), when switching to the collapsed state. This way the last item in the scrollable part is covered by the fixed part of the menu. This is probably a different topic.

Screenshot 2024-08-08 at 16 58 39

@sukhwinder33445 sukhwinder33445 force-pushed the fix/menu-flyout-arrow-position-4594 branch 2 times, most recently from 8f26423 to 2e41d1e Compare August 12, 2024 10:06
@sukhwinder33445
Copy link
Contributor Author

Hi @flourish86,

I have discussed the problem of overflow with Johannes. It shouldn't happen in a normal screen size, and it's a different issue anyway. So for this PR we will leave it as it is.

public/js/icinga/behavior/navigation.js Outdated Show resolved Hide resolved
@nilmerg nilmerg merged commit cc9572d into main Aug 14, 2024
22 checks passed
@nilmerg nilmerg deleted the fix/menu-flyout-arrow-position-4594 branch August 14, 2024 14:29
@nilmerg nilmerg added this to the 2.12.2 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/javascript Affects the javascript framework bug Something isn't working cla/signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation menu hover arrow misplassed
3 participants