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

Side navigation bug fixes #910

Conversation

GCHQ-Developer-847
Copy link
Contributor

Summary of the changes

For #739: Updated CSS so that the visibility is handled by just the 'menu-visibility-visible' class instead of in the media query, and remove unused classes in ic-navigation-group CSS.

For #885: Fixed padding so content is pushed to the side the correct amount with static variant.

For #896: Updated transition handlers - re-added functionality for adding / removing classes on transition start.

Related issue

#739, #885 and #896.

Checklist

  • I have added relevant unit and visual regression tests.
  • I have manually accessibility tested any changes, if relevant.
  • I have ensured any changes match the Figma component library.

@github-actions
Copy link
Contributor

@GCHQ-Developer-847 GCHQ-Developer-847 force-pushed the bugfix/739-empty-side-nav-when-changing-screen-widths branch from a36fe8d to fc277a9 Compare July 17, 2023 20:04
Copy link
Contributor

@jd239 jd239 left a comment

Choose a reason for hiding this comment

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

Only one comment. Good work on the side navigation! :)

private animateCollapsedIconLabels = () => {
this.transitionEndHandler();
this.transitionHandler("start");
this.transitionHandler("end");
Copy link
Contributor

@jd239 jd239 Jul 18, 2023

Choose a reason for hiding this comment

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

Can use your new transitionEndHandler method here..or if it's only being used once, delete the new method.

@GCHQ-Developer-847 GCHQ-Developer-847 force-pushed the bugfix/739-empty-side-nav-when-changing-screen-widths branch 2 times, most recently from a995f43 to da9863b Compare July 18, 2023 10:23
… content is correct

Update side navigation CSS so that the visibility is handled by just the 'menu-visibility-visible'
class instead of in the media query, remove unused classes in ic-navigation-group CSS.

.#739
Fix padding on static side navigation variant so content is pushed to the side the correct amount.

.#885
…ith collapsed labels variant

Update side navigation transition handlers to fix bug with collapsed labels variant not showing
navigation items - re-add functionality for adding / removing classes on transition start.

.#896
@GCHQ-Developer-847 GCHQ-Developer-847 force-pushed the bugfix/739-empty-side-nav-when-changing-screen-widths branch from da9863b to e70779a Compare July 18, 2023 13:21
@GCHQ-Developer-847 GCHQ-Developer-847 merged commit 81cb0e9 into develop Jul 18, 2023
4 checks passed
@GCHQ-Developer-847 GCHQ-Developer-847 deleted the bugfix/739-empty-side-nav-when-changing-screen-widths branch July 18, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants