feat(telemetry): add rich tooltip and improve accessibility#16
feat(telemetry): add rich tooltip and improve accessibility#16
Conversation
…otes - Update `getProbabilityData` in `data/telemetry.ts` to include `event` and `subjective` fields. - Create `CustomTooltip` component in `components/TelemetryTimeline.tsx` to display rich data. - Update `TelemetryTimeline` to use `CustomTooltip`. - Fix table header accessibility by adding `scope="col"`. - Use `TooltipContentProps` for correct typing with Recharts. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @igor-holt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience and accessibility of the Telemetry Timeline chart. By introducing a custom tooltip, users can now gain deeper insights into telemetry events directly from the visualization, seeing not just probabilities but also event details and subjective descriptions. Concurrently, accessibility for tabular data has been enhanced, ensuring a more inclusive experience for all users. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request enhances the Telemetry Timeline with a detailed custom tooltip and improves table accessibility. While the functional additions are valuable, the implementation relies heavily on inline styles and hardcoded colors, which affects maintainability. There are also inconsistencies in data formatting (dates and probabilities) and a potential edge case in the tooltip's conditional logic that should be addressed to ensure a robust and polished user experience.
| } from 'recharts'; | ||
|
|
||
| const CustomTooltip = ({ active, payload, label }: TooltipContentProps<number, string>) => { | ||
| if (active && payload && payload.length && label) { |
There was a problem hiding this comment.
The check && label will evaluate to false if the label is 0. Since the label represents a timestamp (number), it is safer to check for null or undefined explicitly to avoid hiding the tooltip for data points at the start of the Unix epoch.
| if (active && payload && payload.length && label) { | |
| if (active && payload && payload.length && label !== undefined && label !== null) { |
| <div style={{ | ||
| backgroundColor: '#fff', | ||
| border: '1px solid #e0e0e0', | ||
| borderRadius: '8px', | ||
| padding: '12px', | ||
| boxShadow: '0 4px 6px rgba(0, 0, 0, 0.1)', | ||
| maxWidth: '300px' | ||
| }}> |
| maxWidth: '300px' | ||
| }}> | ||
| <p style={{ margin: 0, fontSize: '0.9rem', color: '#666' }}> | ||
| {new Date(label).toUTCString()} |
There was a problem hiding this comment.
The date in the tooltip is formatted using toUTCString(), while the X-axis uses toLocaleDateString(). Using different time zones or formats can be confusing for users. Consider using a consistent formatting method, such as toLocaleString().
| {new Date(label).toUTCString()} | |
| {new Date(label).toLocaleString()} |
| {data.event} | ||
| </p> | ||
| <p style={{ margin: 0, fontSize: '0.9rem' }}> | ||
| Probability: <span style={{ fontWeight: 'bold' }}>{data.probability}</span> |
There was a problem hiding this comment.
The probability value is currently displayed as a raw decimal. Formatting it as a percentage (e.g., 61%) would improve readability and provide a better user experience.
| Probability: <span style={{ fontWeight: 'bold' }}>{data.probability}</span> | |
| Probability: <span style={{ fontWeight: 'bold' }}>{(data.probability * 100).toFixed(0)}%</span> |
This PR enhances the Telemetry Timeline chart by adding a custom tooltip that displays not just the probability, but also the event name and the "subjective" description from the telemetry data. This adds a layer of "delight" and context to the visualization. Additionally, it improves accessibility by adding
scope="col"to the table headers.Changes:
data/telemetry.ts: exposedeventandsubjectiveingetProbabilityData.components/TelemetryTimeline.tsx: implementedCustomTooltipand updatedLineChart.Verification:
pnpm build).PR created automatically by Jules for task 17694988562313522725 started by @igor-holt