Skip to content

feat(vertical-menu): implements updates required for Global Nav v2 #7298

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 8, 2025
Merged

feat(vertical-menu): implements updates required for Global Nav v2 #7298

merged 1 commit into from
May 8, 2025

Conversation

damienrobson-sage
Copy link
Contributor

Proposed behaviour

This PR adds a new component under Vertical Menu: ResponsiveVerticalMenu. This is a two-column menu which is powered by React's Context API and manages complex focus ordering and depth awareness. It is also responsive, switching to a modal sidebar whilst maintaining functionality and structure.

Current behaviour

There is currently no vertical menu option available to consumers that supports complex menu structures with many items.

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Testing instructions

A Storybook example has been provided under VerticalMenu > Responsive > Test that can be used to test the functionality.

@damienrobson-sage damienrobson-sage self-assigned this Apr 11, 2025
@damienrobson-sage damienrobson-sage requested review from a team as code owners April 11, 2025 09:48
@edleeks87 edleeks87 marked this pull request as draft April 11, 2025 09:56
@edleeks87 edleeks87 self-requested a review April 11, 2025 09:57
@damienrobson-sage damienrobson-sage force-pushed the FE-7094-verticalmenu-updates branch from 0689c2c to 630ef4f Compare April 11, 2025 14:30
edleeks87
edleeks87 reviewed Apr 15, 2025
View reviewed changes
Copy link
Contributor

@edleeks87 edleeks87 left a comment

Choose a reason for hiding this comment

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

Good work @damienrobson-sage, overall happy with the approach and most comments I've left are non-blocking. I've also noticed an overflow issue (see below) and I've asked @harpalsingh and @tempertemper to clarify the keyboard behaviour as it seems when you open the submenu you are trapped in a loop between the parent and children items so i just want to clarify if that's correct.

image

2025-04-15 15 20 45

@edleeks87 edleeks87 marked this pull request as ready for review April 15, 2025 14:23
@edleeks87 edleeks87 marked this pull request as draft April 15, 2025 14:27
edleeks87
edleeks87 reviewed Apr 16, 2025
View reviewed changes
...enu/responsive-vertical-menu/responsive-vertical-menu-item/responsive-vertical-menu-item.tsx Outdated
import { IncreaseDepth, useDepth } from "../__internal__/depth.context";
import { useMenuFocus } from "../__internal__/focus.context";

export interface ResponsiveVerticalMenuItemProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the styling match when they pass a Button, wouldn't it just have the current Button styling rather than look like an ResponsiveVerticalMenuItem? There's also the scenario where they set an href so the link/item is focusable but has a focusable Button child; admittedly that's up to the implementation teams to avoid if this approach is agreeable, but in MenuItem we support passing href and onClick so that if they set both there's no WCAG violations etc

edleeks87
edleeks87 previously approved these changes Apr 17, 2025
View reviewed changes
tomdavies73
tomdavies73 reviewed Apr 17, 2025
View reviewed changes
tomdavies73
tomdavies73 reviewed Apr 17, 2025
View reviewed changes
Copy link
Contributor

@tomdavies73 tomdavies73 left a comment

Choose a reason for hiding this comment

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

Well done @damienrobson-sage on getting this done so well in such a short space of time, approach is spot on 👍

tomdavies73
tomdavies73 previously approved these changes Apr 23, 2025
View reviewed changes
@damienrobson-sage damienrobson-sage force-pushed the FE-7094-verticalmenu-updates branch from 6c120ce to 184353a Compare April 23, 2025 10:28
edleeks87
edleeks87 reviewed Apr 23, 2025
View reviewed changes
...sive-vertical-menu/responsive-vertical-menu-item/responsive-vertical-menu-item.component.tsx Outdated
data-role="responsive-vertical-menu-expander-icon"
depth={depth}
expanded={expanded || isActive}
reduceMotion={reducedMotion || false}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this does this need the || false? The context/hook initialises it as false and I can't see anywhere where it could be set to anything that's not a boolean. If that's not the case then we could do the following to coerce it to a boolean etc

reduceMotion={!!reducedMotion}

@damienrobson-sage damienrobson-sage force-pushed the FE-7094-verticalmenu-updates branch from 184353a to c54548f Compare April 23, 2025 11:03
edleeks87
edleeks87 previously approved these changes Apr 23, 2025
View reviewed changes
tomdavies73
tomdavies73 previously approved these changes Apr 24, 2025
View reviewed changes
@tomdavies73 tomdavies73 marked this pull request as ready for review April 24, 2025 09:07
@damienrobson-sage damienrobson-sage force-pushed the FE-7094-verticalmenu-updates branch from 1a232f7 to e7911d7 Compare April 29, 2025 07:32
@damienrobson-sage damienrobson-sage force-pushed the FE-7094-verticalmenu-updates branch from f788e06 to 11dd147 Compare May 1, 2025 10:21
edleeks87
edleeks87 previously approved these changes May 1, 2025
View reviewed changes
@damienrobson-sage damienrobson-sage force-pushed the FE-7094-verticalmenu-updates branch from 4b6fb05 to 3db94f1 Compare May 1, 2025 13:50
@damienrobson-sage damienrobson-sage force-pushed the FE-7094-verticalmenu-updates branch 2 times, most recently from 5b7937e to 42b3070 Compare May 2, 2025 10:46
@harpalsingh
Copy link
Member

Keyboard behaviour working as intended so approving a11y review as well.

edleeks87
edleeks87 previously approved these changes May 7, 2025
View reviewed changes
@harpalsingh
Copy link
Member

Had a 2nd review with the scrollbars for responsive layout and happy with implementation, will review in the future to customise a visual for the darker layout.

@damienrobson-sage damienrobson-sage force-pushed the FE-7094-verticalmenu-updates branch from 314a626 to b04c85b Compare May 7, 2025 14:47
@damienrobson-sage damienrobson-sage merged commit 93828e9 into master May 8, 2025
28 checks passed
@damienrobson-sage damienrobson-sage deleted the FE-7094-verticalmenu-updates branch May 8, 2025 12:24
@carbonci
Copy link
Collaborator

carbonci commented May 8, 2025

🎉 This PR is included in version 154.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edleeks87 edleeks87 edleeks87 approved these changes

@stephenogorman stephenogorman stephenogorman approved these changes

@tomdavies73 tomdavies73 tomdavies73 approved these changes

Assignees

@damienrobson-sage damienrobson-sage

Labels
Passed a11y Review Passed QA Passed Review Passed UX QA released Squad Front End vertical-menu
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@damienrobson-sage @harpalsingh @carbonci @edleeks87 @stephenogorman @tomdavies73