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

MegaMenu: Fixes mega menu showing scroll indicator when it shouldn't #65452

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Mar 28, 2023

Fixes #65394

Noticed this hover indicator in the MenuMenu last couple of days.

This PR fixes

  • Make sure the content expands to full height so we don't show the scroll indicator when we don't need to
  • Fixes the position of the close button for mobile views (was not aligned with the topnav top section). Also adds a border bottom to the mega menu mobile header (so that it looks the same as the normal bottom half of topnav)

This is the bug I am seeing
Screenshot from 2023-03-28 15-32-50

Not sure if we should backport this or not? (as topnav is being rolled out more and more to cloud in 9.4.x ?)

@torkelo torkelo requested a review from a team as a code owner March 28, 2023 13:48
@torkelo torkelo requested review from joshhunt and ashharrison90 and removed request for a team March 28, 2023 13:48
@torkelo torkelo added this to the 9.5.0 milestone Mar 28, 2023
@torkelo torkelo added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Mar 28, 2023
@torkelo
Copy link
Member Author

torkelo commented Mar 28, 2023

Not sure if we should backport this or not? (as topnav is being rolled out more and more to cloud in 9.4.x ?)

@joshhunt
Copy link
Contributor

Fixes #65394 !! 🥳

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/pr-codeql-analysis-javascript.yml:analyze. As part of the setup process, we have scanned this repository and found 27 existing alerts. Please check the repository Security tab to see all alerts.

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

padding looks good 👍

but these changes regress showing the scroll indicator in mobile view. with the mobile header, setting height: 100% makes it taller than the screen so it doesn't work anymore 😢

i've never been able to repro the bug with the scroll indicator so i can't tell if this fixes it or not... i really hope it does 🤞 are there any specific steps you needed to take to see it? 🤔

@torkelo
Copy link
Member Author

torkelo commented Mar 28, 2023

but these changes regress showing the scroll indicator in mobile view. with the mobile header, setting height: 100% makes it taller than the screen so it doesn't work anymore

@ashharrison90 ah, I was expecting that (and tried to replicate it but could not). will fix

@ashharrison90
Copy link
Contributor

also re: backporting - i think we should, same as we would for any other bugfix 👍

@torkelo torkelo added backport v9.4.x Mark PR for automatic backport to v9.4.x add to changelog and removed no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Mar 28, 2023
@torkelo torkelo modified the milestones: 9.5.0, 9.4.x Mar 28, 2023
@grafanabot
Copy link
Contributor

Hello @torkelo!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@torkelo
Copy link
Member Author

torkelo commented Mar 28, 2023

@ashharrison90 fixed the mobile issue, was easy standard flex fix :)

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

looks great! 🥳

hope it fixes the issue for everyone 🤞

@torkelo torkelo merged commit 273e05a into main Mar 28, 2023
@torkelo torkelo deleted the fix-mega-menu-scroll-indicator branch March 28, 2023 15:47
grafanabot pushed a commit that referenced this pull request Mar 28, 2023
…65452)

* MegaMenu: Fixes mega menu showing scroll indicator when it shouldn't

* fixing css

(cherry picked from commit 273e05a)
torkelo added a commit that referenced this pull request Mar 28, 2023
…houldn't (#65465)

MegaMenu: Fixes mega menu showing scroll indicator when it shouldn't (#65452)

* MegaMenu: Fixes mega menu showing scroll indicator when it shouldn't

* fixing css

(cherry picked from commit 273e05a)

Co-authored-by: Torkel Ödegaard <torkel@grafana.com>
@zerok zerok modified the milestones: 9.4.x, 9.4.8, 9.5.0 Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Down arrow appearing when it shouldn't while using topnav
5 participants