fix(interaction): 🐛 create conditional Xplacement based on container width #1268
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| x: foldContainerSize<Xplacement>(chartWidth)({ | ||
| xs: (w) => (xAnchor < w * 0.5 ? "right" : "left"), | ||
| md: (w) => (xAnchor < w * 0.25 ? "right" : "left"), | ||
| }), |
There was a problem hiding this comment.
I feel like the logic is a bit brittle, we are never sure that this will work correctly right & we must rely on the > 0.5 & 0.25 heuristics. For me it would be better the Tooltip from MUI here.
- It is already included in the bundle (see datatable file)
- It will correctly check (through Popper) the boundary of the container and position the Tooltip accordingly
- It would allow us to remove a lof the code here.
There was a problem hiding this comment.
my resistance to replacing the Tooltip with the MUI version is that the two APIs work very differently with the MUI wrapping the target, and the current one taking the position from the state of useInteraction(). I think this is mostly in order to be able to follow the cursor in some instances, and in others to lock to something like a bar or point in a chart.
Seeing as the Tooltip is used in all 13 chart types with multiple variants and conditional logic, I think the effort to replace all the behaviours with the MUI version will take considerable effort (especially from someone new to the project)
There was a problem hiding this comment.
Fair point, you're right 👍
In the future, I would guess but not sure that we could make either Popper or Tooltip work directly maybe without wrapping.
Nit: For what it's worth, I had trouble understanding the fold code at first glance. I find that we could simplify the logic a bit.
const isSmall = containerWidth < 600;
const leftThreshold = (isSmall ? 0.5 : 0.25) * containerWidth
const placement = xAnchor < leftThreshold ? 'right' : 'left'
There was a problem hiding this comment.
i agree that when its a binary option like at the moment with small and larger screens then the fold seems excessive, but I want to make sure the solution is robust, so in the future (or if there needs to be edge cases) then its possible to add more options without the need for a switch:
x: foldContainerSize<Xplacement>(chartWidth)({
xs: (w) => (xAnchor < w * 0.5 ? "right" : "left"), // places tooltip on opposite side of screen
md: (w) => (xAnchor < w * 0.25 ? "right" : "left"), // on medium size charts, places based on 1/4 ratio
lg: (w) => "right", // on large size charts, places always on right
}),There was a problem hiding this comment.
OK, fair and my point is a nit. I think that shaping the code so that breakpoints (especially more than 2) are used in the future should not be a goal, as usually I find breakpoints make things more brittle than sturdy.
Goals/Scope
After investigating it seems the bug is cased on small containers where the tooltip size is larger then the allotted space. Since larger screens still work, I looked to create a solution that could conditionally place the tooltip based on the charts size rather then measuring the window.
Description
Created a fold function that accepts the
chartWidthand returns a function that can be used to define the behaviour of the xPlacement depending on that size. Here we can define thexs(smallest) andmd(@breakpoint 600) to define the mobile-first behaviours.Comments
I'm not sure how to test the embedded links without deploying this to INT, any suggestions are welcome in the comments 👇