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

Tweak navbar overflow logic to catch selected items #11778

Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Jan 23, 2024

Summary

  • Updates nav link in navbar check to ensure the link is completely bounded by the navbar
  • If not, it is included in the dropdown menu
  • Also skips attempting to navigate if we are already on the selected page from the dropdown menu

References

Fixes #11776

Reviewer guidance

Test on a 375 pixel wide screen, click into the info page on the device tab.
Ensure that the navigation overflow is still visible.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

Check that link is completely bound by nav container to check it is visible.
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Thanks @rtibbles - manual QA checks out on the additional improvements and the redundant nav logic. There might be some additional funky edge cases when resizing small browser screens and doing lots of navigation in between resizing, but as we discussed separately on slack, I think for now, it's okay to wait on that. We can consider all of the complex edge cases as we start thinking about moving some of the responsibility of this into a KComponent.

@marcellamaki marcellamaki merged commit a9fbc6b into learningequality:release-v0.16.x Jan 23, 2024
34 checks passed
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.

When currently selected nav item is in the dropdown menu, the dropdown menu disappears
2 participants