-
Notifications
You must be signed in to change notification settings - Fork 316
feat(chart): add chart tooltip #3302
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
6c9bbf4 to
f01ba24
Compare
| const priceValue = priceData.value; | ||
| const date = new Date(Number(param.time) * 1000); | ||
| const formattedDate = dateFormatter.format(date); | ||
| const formattedPrice = priceFormatter.format(priceValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I see that it might not be possible to use the React Aria component, since we're tying into the Trading View widget's events.
we may be able to still leverage the useTooltip() hook, though, to ensure proper a11y is maintained for it, though 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The price chart itself is a canvas, so I'm not sure what we can actually do here when it comes to a11y. The tooltip kinda assumes the user is able to interact with a price line within the canvas by using their mouse. The tooltip contains no interactive elements.
If anything we should update the aria-label of the <canvas> itself, but yeah...
cprussin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great but let's stick to react & react-aria Tooltip if possible.
Even if react-aria isn't possible, using react to render the tooltip and just having the callback set state that controls the visibility of it should be completely doable without the awkward manual html string interpolation and dropping to manual DOM building.
| tooltip.innerHTML = ` | ||
| <table class="${styles.tooltipTable ?? ""}"> | ||
| <tbody> | ||
| <tr> | ||
| <td colspan="2">${formattedDate}</td> | ||
| </tr> | ||
| <tr> | ||
| <tr> | ||
| <td>Price</td> | ||
| <td>${formattedPrice}</td> | ||
| </tr> | ||
| <tr> | ||
| <td>Confidence</td> | ||
| <td>${confidenceText}</td> | ||
| </tr> | ||
| </tbody> | ||
| </table> | ||
| `; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure you can do this with react & react-aria by using a react-aria Tooltip component, and just use this callback to set state that controls the position & visibility of the tooltip. That would clean up the code considerably and remove a lot of the awkward manual html building, plus you'd get all the accessibility niceness of react-aria.
Summary
In this PR we add a simple tooltip to the price chart.
Rationale
How has this been tested?