Skip to content

[BOOKINGSG-9311][RYN] migrate local nav component#1178

Open
ryan-nguyen-t wants to merge 5 commits into
pre-release/v4from
BOOKINGSG-9311/local-nav
Open

[BOOKINGSG-9311][RYN] migrate local nav component#1178
ryan-nguyen-t wants to merge 5 commits into
pre-release/v4from
BOOKINGSG-9311/local-nav

Conversation

@ryan-nguyen-t
Copy link
Copy Markdown

Checklist

  • Migrated the component styles
    • className is chained correctly with clsx
    • User style prop is set as CSS variable
  • Changes follow the project guidelines in CONVENTIONS_V4.md
    - [ ] Updated Storybook documentation
    - [ ] Added/updated unit tests
  • Added visual tests

@ryan-nguyen-t ryan-nguyen-t self-assigned this May 14, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

eh can exclude the custom content and scroll jumping since that's meant to be done by the consumer. it's not relevant to the DS. we only need to set up a basic example that showcases the sticky nav behaviour

const [selectedItemIndex, setSelectedItemIndex] = useState(0);

return (
<div className="story-row-container story-background">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can exclude the container class since there's only 1 item being rendered

`);
});

test("Keyboard navigation", async ({ story }) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

move keyboard navigation to unit tests

await story.init("menu");
});

test("Default", async ({ story }) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

include the hover state and non-selected state. currently it only covers the item-selected state


test("Default", async ({ story }) => {
await compareScreenshot(story, "mount", {
locator: story.locators.menu,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

exclude the locators by default. we want to check the default width and height behaviour in most cases


test("Accessibility", async ({ story }) => {
await expect(story.locators.dropdown).toMatchAriaSnapshot(`
- menu:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

check the expanded state here as well

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this doesn't look right. the dropdown should be fixed to the top of the page

.click();
});

await test.step("Scroll until sticky", async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

include a snapshot for the sticky unopened dropdown

});

test("Sticky on scroll", async ({ story }) => {
await test.step("Select an item", async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

include a snapshot for the non-sticky opened dropdown

});
});

test("Sticky on scroll", async ({ story }) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hover the hover states


export const navWrapperStickied = css`
.${navSelect} {
margin: 0 calc(-1 * var(${tokens.navWrapper.sideMargin}, 0px));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

update all for consistency

Suggested change
margin: 0 calc(-1 * var(${tokens.navWrapper.sideMargin}, 0px));
margin: 0 calc(var(${tokens.navWrapper.sideMargin} * -1px, 0px));

@qroll qroll added the type: chore For technical improvements or refactoring. label May 14, 2026
@qroll qroll added this to the v4.0.0-alpha.3 milestone May 14, 2026
Comment on lines +215 to +217
const text =
await story.locators.dropdownLabel.textContent();
return (text ?? "").includes("Title 2");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we have a longer list? I was thinking that since we're only on title 2, it can resulted in zero scrolling or non-sticky result

});
await story.locators.dropdownLabel.click();
await expect(story.locators.dropdownList).toBeVisible();
await compareScreenshot(story, "sticky-open", {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It seems to be missing the scrolling part for this to have a sticky state

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

Labels

type: chore For technical improvements or refactoring.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants