Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Overflowfix #11743

Merged
merged 6 commits into from
Jan 23, 2024
Merged

Overflowfix #11743

merged 6 commits into from
Jan 23, 2024

Conversation

AllanOXDi
Copy link
Member

@AllanOXDi AllanOXDi commented Jan 18, 2024

Summary

Fixes the navigation menu to overflow correctly. There was also another unnecessary scrollbar that could hide the BottomAppBar buttons on smaller screens on the facility plugin, it is also fixed by this PR.
Closes #11423

Before

Screen.Recording.2024-01-12.at.22.58.24.mov

Button being hidden

Screen.Recording.2024-01-15.at.17.37.58.mov

After the fix

Screen.Recording.2024-01-18.at.23.41.21.mov

References

#11423

Reviewer guidance

  • Test if the navlinks are still getting hidden on the device plugins with different screen sizes.
  • Test whether scroll bar is still appearing on the BottomAppBar on the facility plugin
  • And from the code perspective, does it make sense?

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

@github-actions github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: small labels Jan 18, 2024
@AllanOXDi AllanOXDi added the TODO: needs review Waiting for review label Jan 18, 2024
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

I was still able to get into a state where the navigation overflow menu did not appear - it seemed to be because while the navLinkTop was higher than the containerBottom, it still did not fit in the container.

I was able to fix it locally by accounting for the height of the nav bar link as well. I updated the filter function body to this:

              const navLink = this.$refs.navLinks[index].$el;
              const navLinkTop = navLink.offsetTop;
              const navLinkHeight = navLink.clientHeight;

              const containerTop = this.$refs.navContainer.offsetTop;
              const containerBottom = containerTop + this.$refs.navContainer.clientHeight;
              return navLinkTop + navLinkHeight >= containerBottom;

And didn't see any more issues.

@AllanOXDi AllanOXDi requested a review from rtibbles January 23, 2024 20:40
@rtibbles
Copy link
Member

We chatted about this on slack, but just making a note here, that there appears to be a specific interaction between an 'active' nav link being in the dropdown menu, and the drop down menu not appearing at all.

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Have broken out the remaining issue into a follow up issue: #11776

@rtibbles
Copy link
Member

Docs build is fixed on the release branch. Merging.

@rtibbles rtibbles merged commit 65b5722 into learningequality:release-v0.16.x Jan 23, 2024
33 of 34 checks passed
@AllanOXDi AllanOXDi deleted the overflowfix branch February 7, 2024 22:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HorizontalNavigationWithOverflow isn't always providing the overflow correctly
2 participants