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

chore(sticky-header): port refactor to v2 #11773

Merged

Conversation

jkaeser
Copy link
Member

@jkaeser jkaeser commented May 7, 2024

Description

Ports changes made to feat/masthead-v2-dev branch in #11734 to v2 branch.

In order to adequately test the new Sticky Header Sandbox story, I had to clean up the Dotcom Shell stories. The mock secret menu item works again now, and I've reduced a good amount of bloat in the stories' markup. While I was at it, I noticed I never updated Dotcom Shell's navLinks prop for L0 items to Masthead's new l0Data prop, so I added support for that as well.

Testing

The code changes here are mostly identical to the aforementioned PR, with the only divergences being changes necessary for v2. Please verify each of the Dotcom Shell stories works as expected, taking particular care to test the sticky elements behaviors on both desktop and mobile.

…e is also a ToC (carbon-design-system#11734)

none

Given the page uses both an L1 menu and a table of contents (ToC),
and the page is displaying at a mobile breakpoint,
and the ToC menu is the sticky element (i.e. stuck to the top of the viewport),
and the user scrolls up slightly to reveal the L1 dropdown,
and the user open the L1's dropdown menu,
then the dropdown menu gets hidden above the viewport, forcing the user to scroll up more to access it.
Instead, the top of L1 dropdown menu should be visible when opened.

This problem is occurring because the dropdown exists in the document flow, and the StickyHeader utility doesn't reset stuck elements' positions when the menu is opened.

The easiest solution I found was to position the dropdown menu absolutely like most other dropdown menus you might encounter. This bypasses the tricky problem of trying to get the StickyHeader to recalculate, and in my opinion is a slight enhancement in both performance (avoids layout shifts and associated repaint costs) and overall UX (gives dropdown a more expected behavior).

**Changed**

- Prevents mobile L1 dropdown menu from becoming hidden above viewport when opened while there is currently a sticky table of contents menu.
- Prevents L1 from scrolling away when a submenu is open on desktop or the dropdown is open on mobile.
- Consolidates StickyHeader utility tracked data to make debugging easier.
- Refactors StickyHeader logic to support all potentially sticky elements being on page at the same time.

To test the main fix:
1. Visit [the Dotcom Shell > With L1 story](https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11734/index.html?path=/story/components-dotcom-shell--with-l-1) in the deploy preview.
2. Change the preview size to "Large mobile" or "Small mobile"
3. Scroll down the page until the ToC is stuck to the top of the viewport.
4. Scroll back up just enough to see the L1.
5. Open the L1 and verify the top of it doesn't jump back up above the viewport.
6. Scroll down and verify the L1 stays stuck to the top of the viewport.
7. Close the L1 and verify the ToC gets stuck to the top of the viewport as you scroll down.

You should also test the L1 remains stuck when open at desktop widths.

This PR also changes a good amount of the StickyHeader class logic. To support regression testing the existing sticky element behaviors, I've added the [Sticky Element Sandbox story](https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11734/index.html?path=/story/components-dotcom-shell--sticky-element-sandbox). Test various knob configurations at both desktop and mobile viewport sizes and let me know how you were able to break it 😄

**Note:** If you find the sticky elements aren't working correctly when switching between stories, try reloading the window.
@jkaeser jkaeser added package: utilities Work necessary for the Carbon for IBM.com utilities package package: styles Work necessary for the Carbon for IBM.com styles package package: web components Work necessary for the IBM.com Library web components package owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants v2 labels May 7, 2024
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 7, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 7, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 7, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 7, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 7, 2024

@jkaeser jkaeser marked this pull request as ready for review May 8, 2024 19:02
@jkaeser jkaeser requested a review from a team as a code owner May 8, 2024 19:02
@jkaeser jkaeser requested review from kennylam and andy-blum and removed request for a team May 8, 2024 19:02
@jkaeser jkaeser marked this pull request as draft May 10, 2024 16:22
@jkaeser jkaeser marked this pull request as ready for review May 14, 2024 13:49
@jkaeser
Copy link
Member Author

jkaeser commented May 14, 2024

I was seeing some strange behavior at mobile viewport resolutions while using Chrome's dev tools. Stuck elements were being positioned something like 40 pixels above the top viewport edge. However, I do not see the same issue when simply resizing my window down, nor do I see it in Firefox or Safari.

It's making me nervous, though, so I'd appreciate if reviewers could test the mobile sticky behaviors quite thoroughly!

@jkaeser jkaeser force-pushed the chore/port-stickyheader-refactor-v2 branch from 2b1b9a7 to 4d0f1ec Compare May 16, 2024 19:24
Copy link
Member

@andy-blum andy-blum left a comment

Choose a reason for hiding this comment

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

All the dotcom-shell stories look good to me

Copy link
Contributor

@m4olivei m4olivei left a comment

Choose a reason for hiding this comment

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

LGTM!

@jkaeser jkaeser added Ready to merge Label for the pull requests that are ready to merge and removed 👀 eyes needed labels May 23, 2024
@m4olivei
Copy link
Contributor

Looks like Percy isn't happy about a missing snapshot? Doesn't seem releated this this...

@jkaeser
Copy link
Member Author

jkaeser commented May 28, 2024

Looks like Percy isn't happy about a missing snapshot? Doesn't seem releated this this...

I'll try closing & reopening the PR to re-run the job first. If that doesn't work, time to call in a Carbon team member.

@jkaeser jkaeser closed this May 28, 2024
@jkaeser jkaeser reopened this May 28, 2024
@kodiakhq kodiakhq bot merged commit 365c9b6 into carbon-design-system:main May 28, 2024
14 of 21 checks passed
@jkaeser jkaeser deleted the chore/port-stickyheader-refactor-v2 branch May 28, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants package: styles Work necessary for the Carbon for IBM.com styles package package: utilities Work necessary for the Carbon for IBM.com utilities package package: web components Work necessary for the IBM.com Library web components package Ready to merge Label for the pull requests that are ready to merge v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants