refactor: tooltips using CSS solution to JS solution #639
Conversation
…s using js solution" This reverts commit df2588a.
|
@idoshamun I have now created another component to abstract the complexity of working with next/link. I have also minimized the required parameters for the tooltips to make it very simple and approachable (left with 3 parameters: children, content, and placement itself). @rebelchris I have applied the necessary optimizations, thank you so much for the info. Thank you both for the kinds words and the awesomest of reviews! This should be ready for another round now. |
There was a problem hiding this comment.
Wow nice work @sshanzel I have one question open.
But should not be a blocker, it's more a "wishlist" item.
| @@ -91,17 +93,14 @@ export default function MainLayout({ | |||
| : 'non-responsive-header' | |||
| }`} | |||
| > | |||
| <Link | |||
| <LinkWithTooltip | |||
There was a problem hiding this comment.
Just throwing this one out here:
Is there no way to have the SimpleTooltip determine if the direct child is a <Link> and then do something else?
Think it would be super cool if we could just have 1 sole component for the tooltip usage.
If it's not possible, this seems like a good first step!
There was a problem hiding this comment.
Thank you for the review! 🚢
That's a cool idea. Though the part where it got me separating the two is the way we will be handling the trigger element itself. The other one, we will be wrapping the children, and the other one will be utilizing the ref of the actual element since next/link has problems with accepting custom components.
Combining them in a single component might provide quite a bit of complexity (as having multiple conditions whether to apply a ref or not). To keep things simple and easier to understand for each scenario, I had separated them.
|
|
||
| return ( | ||
| <TippyTooltip | ||
| shouldLoad={getShouldLoadTooltip()} |
There was a problem hiding this comment.
I think that instead of shouldLoad, we can add an if statement at the beginning and if should load is false we can return an empty element.
Something like:
if (!shouldLoad) {
return <></>;
}
What do you think?
There was a problem hiding this comment.
That is a good idea. With that, I tried rendering the children only if shouldLoad is false (since it is necessary to be displayed), but tests started failing again even though it still has the aria-label. I also implemented a dynamic rendering since it is optional but it seems to crash again on tests.
|
Doing some manual QA I found an issue with the new tooltip when hovering on the menu button |
|
@idoshamun I have now fixed the clash between the classes of arrows in the |
|
Different placements of the actual tooltips:
|
|
Light mode: |
|
WOOOO! I didn't enable the auto-merge so I can feel the green button on this! 🚀 Thank you for the awesome reviews to you both! 🚢 |
DD-229 #done
After so many trials and errors, I have finally found a workaround in making the tests not fail while using the tooltip libraries that we have listed as our option on our confluence page.
When you literally implement a sample code utilizing the tooltip library, the tests would suddenly fail relating to just rendering some components that weren't even touched by the sample code (kindly check the
wipcommits).But we were persistent, we want to switch from CSS to JS solution for tooltips. Thankfully we have the
dynamicParentto load packages conditionally. In this case, I had to switch off the loading of the actual tooltip library since by just literally loading it, rendering tests would fail.Would switching the tooltip library off on test env affect the desired result for existing tests cases? No, since the important part in finding the actual element in test cases, is the
aria-labelattribute, which we automatically apply on every element wrapped by our dynamicTooltipcomponent, regardless of whether we load the actual tooltip library or not.Note: it can be noticed that there are two ways to implement the tooltip, one is using the actual wrapper that has
dynamicParent. The second one uses arefobject to identify the trigger element.The reason for the second one is due to
next/linkhas an existing issue when wrapping a custom component, so when we use the tooltip as a child and wrap the actual content of thenext/link, it will not work. We have to use its reference to identify the trigger of the actual tooltip. Thankfully, the library we are using supports it out of the box.vercel/next.js#7915
Happy reviewing!