perf(exchange layer): deduplicate exchange tooltips #6594
Conversation
VIKTORVAV99
added
performance 🏎
techdebt
VIKTORVAV99
requested review from
a team and
silkeholmebonnen
and removed request for
a team
|
The tooltip on mobile goes out of the screen sometimes, and it didn't before. It seems like this was already a problem on desktop before, but we might want to change it in this PR as well? |
Nice find! There is another PR open at #6359 that focuses on that for other tooltips so I'll try and get that merged as soon as possible and see if it fixes this as well or can be reused in here. |
| @@ -17,10 +18,27 @@ export default function TooltipWrapper( | |||
| if (!tooltipContent) { | |||
| return children; | |||
| } | |||
| const [isOpen, setIsOpen] = useState(false); | |||
| const { isMobile } = useDimensions(); | |||
There was a problem hiding this comment.
Let's use our existing hook instead: const isBiggerThanMobile = useBreakpoint('sm');
There was a problem hiding this comment.
the sm breakpoint is a lot smaller than what we use as isMobile on the map, should it not be md? Or should we change the map as well?
There was a problem hiding this comment.
Ended up using isMobile and negated the value of the hook (was quicker and less error prone than changing reversing all the logic.
I also used 'md' to match what it was before, but I'm open to changing this.
| if (transform.x - 100 * transform.k > viewportWidth) { | ||
| return null; | ||
| } | ||
| useEffect(() => { |
There was a problem hiding this comment.
Doesn't this break the Rules of Hooks, as the hook is only conditionally rendered? Guess this was also a problem before though, so we should probably use the linting plugin for hooks: https://github.com/facebook/react/tree/main/packages/eslint-plugin-react-hooks
There was a problem hiding this comment.
I do not know but since the renders should not change between the hours themselves and we are re rendering all exchanges I think we should be safe, but I'll look into it.
There was a problem hiding this comment.
I'll leave it as is now and then I'll open a new PR with the new eslint plugin and fix everything that needs to be fixed in there.
Issue
We have a duplicate code that is functioning almost exactly the same and is creating a lot of unnecessary dom elements of which half are hidden at any given time.
Description
This PR deduplicates the code and unifies the MobileExchangeTooltip and ExchangeTooltip as well as ExchangeTooltipWrapper and MobileExchangeTooltipWrapper into just one single component respectively that can be used by both mobile and none mobile.
Some highlights:
Double check
poetry run test_parser "zone_key"pnpx prettier@2 --write .andpoetry run formatin the top level directory to format my changes.