fix: user hub placement

[joanna-pagepro]

Description

Fix for transaction pending button overlapping issue

Testing

• Step 1. Create an action, so that the side panel is open.
• Step 2. Click on the UserHub.
• Step 3. Click on the Activate button.
• Step 4. Activate some tokens.
• Step 5. Check if the transaction pending button is still overlapping.

Diffs

New stuff ✨

Changes 🏗

• blur-related styles are now added to elements with modal-blur or modal-blur-navigation classes
• there's no duplication of the user navigation component on the desktop view - the same component is shown when a modal is open and closed
• The user navigation component is moved to the top when the shouldShowHeader prop is passed to the Modal component

Deletions ⚰️

TODO

• Add TODOs

Resolves #2456

[melyndav]

Hey @joanna-pagepro 🦖

When I attempt to test this, the userhub closes the side panel and then automatically closes straight after as shown in the recording below:

[joanna-pagepro]

@melyndav please review it again

[melyndav]

Thanks for that fix @joanna-pagepro, I've been able to test it and everything is looking resolved now. nice work! 👍

[mmioana]

Cool work @joanna-pagepro ✨
I didn't manage to reproduce the issue anymore 👍 however I left some code refactoring comments

[mmioana]

Can we please refactor document.querySelector in a utils function?

const getElementWithSelector = (selector) => document.querySelector(selector);

and use it here const navigationWrapper = getElementWithSelector('.modal-blur-navigation')

[joanna-pagepro]

What is the advantage of doing so?

[mmioana]

The advantage would be to abstract the way elements are selected.
In case additional logic will be needed in the future, eg. log an error if the element is not found for a specific selector, only the utility function will need to be updated.

[mmioana]

Can we extract this in a hook?

const useAddClassToElement = ({ shouldAddClass, element, className }) => {

 useEffect(() => {
    if (
      shouldAddClass &&
      !element.classList.contains(className)
    ) {
      element.classList.add(className);
    }

    return () => {
      element.classList.remove(className);
    };
  }, [shouldAddClass, element, className]);

}

And use it here as

useAddClassToElement({ 
  shouldAddClass: isOpen && shouldShowHeader,
  className: 'show-header-in-modal',
  element: document.body
})

[mmioana]

Can we please refactor this part navigationWrapper && navigationWrapper.contains(element) into an utils function?

const isChildOf = (parent, child) => parent && parent.contains(child);

and use it here (isChildOf(navigationWrapper, element) && !isTablet)

[mmioana]

Is anything stopping us from using z-20?

[joanna-pagepro]

I just think that 11 is enough here. I haven't tested it with 20

[iamsamgibbs]

Just dropping a note to say it is preferred to use the z-index values in tailwind.config.js rather than arbitrary values.

[mmioana]

In this case, we could add these values (11 and 1001) in tailwind.config.js if the already defined values are not usable in this scenario

[joanna-pagepro]

@mmioana can you check the changes and my answers?

[mmioana]

Thank you @joanna-pagepro for your prompt changes! 💮
Could please tackle also the other two comments?

[joanna-pagepro]

@mmioana done

[mmioana]

Thank you @joanna-pagepro for your work! 🚀 💟

[melyndav]

Re-approving this one @joanna-pagepro! Thanks again for your work. 🤩