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

feat: add new sticky component to handle stacked stickies #5088

Merged
merged 7 commits into from
Oct 19, 2023
Merged

Conversation

nunogois
Copy link
Member

@nunogois nunogois commented Oct 18, 2023

https://linear.app/unleash/issue/2-1509/discovery-stacked-sticky-elements

Adds a new Sticky element that will attempt to stack sticky elements in the DOM in a smart way.
This needs a wrapping StickyProvider that will keep track of sticky elements.

This PR adapts a few components to use this new element:

  • DemoBanner
  • FeatureOverviewSidePanel
  • DraftBanner
  • MaintenanceBanner
  • MessageBanner

Pre-existing top properties are taken into consideration for the top offset, so we can have nice margins like in the feature overview side panel.

Before - Sticky elements overlap 😞

image

After - Sticky elements stack 😄

image

@vercel
Copy link

vercel bot commented Oct 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 19, 2023 2:45pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Oct 19, 2023 2:45pm

const registerStickyItem = useCallback(
(item: RefObject<HTMLDivElement>) => {
setStickyItems((prevItems) => {
if (item && !prevItems.includes(item)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a non trivial logic without tests

Copy link
Member Author

@nunogois nunogois Oct 19, 2023

Choose a reason for hiding this comment

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

Can you please clarify the part that you find non-trivial? The line on this comment is a simple "add if not added yet".
Anyways, added some tests: eb8a0aa. Let me know if there are any specific tests you would like to see.

I agree that it's a non-trivial issue to fix overall, and it can also be non-trivial to write tests for, since it's expecting a real DOM, in a browser environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Non trivial UI logic here is:

  • checking includes
  • adding to the end of the list
  • sorting
  • checking presence of both elements
  • subtraction
    If I change any of those I'd expect my tests to catch me by hand.
    We can separate sorting algorithm from DOM and unit test the sorting independently.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW if the code will tell it's difficult to do then I wouldn't proceed with the refactoring.


const { registerStickyItem, unregisterStickyItem, getTopOffset } = context;

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about combining useEffects?

    useEffect(() => {
        if (!ref.current) return;

        if (initialTopOffset === null) {
            const topValue = parseInt(getComputedStyle(ref.current).getPropertyValue('top'), 10) || 0;
            setInitialTopOffset(topValue);
        }

        setTop(getTopOffset(ref) + (initialTopOffset || 0));

        registerStickyItem(ref);

        return () => unregisterStickyItem(ref);
    }, [getTopOffset, initialTopOffset, registerStickyItem, unregisterStickyItem]);

Copy link
Member Author

Choose a reason for hiding this comment

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

Could potentially work. Something like:

useEffect(() => {
    if (!ref.current) {
        return;
    }

    // We should only set the initial top offset once - when the component is mounted
    // This value will be set based on the initial top that was set for this component
    // After that, the top will be calculated based on the height of the previous sticky items + this initial top offset
    if (initialTopOffset === null) {
        setInitialTopOffset(
            parseInt(getComputedStyle(ref.current).getPropertyValue('top')),
        );
    }

    // We should register the sticky item when it is mounted...
    registerStickyItem(ref);

    // (Re)calculate the top offset based on the sticky items
    setTop(getTopOffset(ref) + (initialTopOffset || 0));

    // ...and unregister it when it is unmounted
    return () => {
        unregisterStickyItem(ref);
    };
}, [ref, registerStickyItem, unregisterStickyItem, getTopOffset]);

But I think I prefer having the separation of concerns, at least in this first iteration.

Copy link
Contributor

@kwasniew kwasniew left a comment

Choose a reason for hiding this comment

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

Nicely done. That's a tricky library component to write.

@nunogois nunogois merged commit 347c1ca into main Oct 19, 2023
12 checks passed
@nunogois nunogois deleted the feat-sticky branch October 19, 2023 14:50
nunogois added a commit that referenced this pull request Oct 23, 2023
Tiny zIndex fix for the draft banner for a regression introduced in
#5088

### Before - Draft banner is displayed on top of the profile popup:

![image](https://github.com/Unleash/unleash/assets/14320932/63865f01-9bbe-42f1-9cc4-85c3c985334c)

### After - Profile popup displays on top of the draft banner:

![image](https://github.com/Unleash/unleash/assets/14320932/565e1017-5163-445d-bc0c-ee957023241b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants