🎨 Palette: Enhanced Telemetry Tooltip & Table A11y#26
Conversation
💡 What: Added a custom tooltip to the Telemetry Timeline chart to display detailed event info (Event, Outcome, Subjective) and improved the data table accessibility by adding scope attributes and a caption. 🎯 Why: Users needed to cross-reference the table to understand chart spikes. Screen reader users lacked context in the data table. ♿ Accessibility: Added scope="col" to table headers and a descriptive caption. 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 visualization. It addresses the cognitive load associated with interpreting chart spikes by providing richer contextual data directly within the chart's tooltip. Concurrently, it enhances the data table's usability for screen reader users by implementing standard accessibility attributes, making the displayed information more structured and understandable. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a custom tooltip for the telemetry timeline chart, providing more detailed event information, and enhances the accessibility of the accompanying data table. The implementation is solid, but I have a couple of suggestions for the TelemetryTimeline.tsx component to improve type safety and maintainability by using proper TypeScript types and avoiding hardcoded values.
| } from 'recharts'; | ||
|
|
||
| // Custom Tooltip for enhanced details | ||
| const CustomTooltip = ({ active, payload, label }: any) => { |
There was a problem hiding this comment.
Using any for component props bypasses TypeScript's type safety. You can use an inline import to leverage recharts's own TooltipProps type for better type safety and developer experience. This will correctly type the active, payload, and label props.
| const CustomTooltip = ({ active, payload, label }: any) => { | |
| const CustomTooltip = ({ active, payload, label }: import('recharts').TooltipProps<number, string>) => { |
| <p style={{ fontWeight: 'bold', marginBottom: '5px' }}> | ||
| {label ? new Date(label).toUTCString() : ''} | ||
| </p> | ||
| <p style={{ color: '#8884d8' }}>Probability: {data.probability}</p> |
There was a problem hiding this comment.
The color #8884d8 is hardcoded here. This same color is used for the chart's line stroke on line 59. To improve maintainability and ensure consistency, consider defining this color as a constant and reusing it in both places. This makes future theme or color changes much easier.
There was a problem hiding this comment.
Pull request overview
This PR enhances the telemetry visualization by adding a custom tooltip that displays detailed event information (Event, Outcome, Subjective) alongside probability values, reducing the need for users to cross-reference the data table. Additionally, it improves the accessibility of the data table by adding scope="col" attributes to table headers and a descriptive caption for screen readers.
Changes:
- Added custom tooltip component to display comprehensive event details in the chart
- Extended data export to include event, outcome, and subjective fields for tooltip consumption
- Enhanced table accessibility with proper scope attributes and caption element
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | New lockfile generated with all dependencies including eslint packages |
| package.json | Added eslint and eslint-config-next as devDependencies |
| data/telemetry.ts | Extended getProbabilityData() to include event, outcome, and subjective fields |
| components/TelemetryTimeline.tsx | Implemented CustomTooltip component and improved table accessibility |
| .Jules/palette.md | Added design journal documenting learnings about contextual visualization and accessibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Custom Tooltip for enhanced details | ||
| const CustomTooltip = ({ active, payload, label }: any) => { | ||
| if (active && payload && payload.length) { | ||
| const data = payload[0].payload; | ||
| return ( | ||
| <div className="card" style={{ padding: '10px', border: '1px solid #ccc' }}> | ||
| <p style={{ fontWeight: 'bold', marginBottom: '5px' }}> | ||
| {label ? new Date(label).toUTCString() : ''} | ||
| </p> | ||
| <p style={{ color: '#8884d8' }}>Probability: {data.probability}</p> | ||
| <p>Event: {data.event}</p> | ||
| <p>Outcome: {data.outcome}</p> | ||
| <p style={{ fontStyle: 'italic', marginTop: '5px', fontSize: '0.9em', color: '#555' }}> | ||
| "{data.subjective}" | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
The tooltip uses inline styles throughout. Consider extracting these styles to CSS classes or a styled component for better maintainability and consistency with the rest of the codebase.
| // Custom Tooltip for enhanced details | |
| const CustomTooltip = ({ active, payload, label }: any) => { | |
| if (active && payload && payload.length) { | |
| const data = payload[0].payload; | |
| return ( | |
| <div className="card" style={{ padding: '10px', border: '1px solid #ccc' }}> | |
| <p style={{ fontWeight: 'bold', marginBottom: '5px' }}> | |
| {label ? new Date(label).toUTCString() : ''} | |
| </p> | |
| <p style={{ color: '#8884d8' }}>Probability: {data.probability}</p> | |
| <p>Event: {data.event}</p> | |
| <p>Outcome: {data.outcome}</p> | |
| <p style={{ fontStyle: 'italic', marginTop: '5px', fontSize: '0.9em', color: '#555' }}> | |
| "{data.subjective}" | |
| </p> | |
| </div> | |
| import styled from 'styled-components'; | |
| const TooltipContainer = styled.div.attrs({ className: 'card' })` | |
| padding: 10px; | |
| border: 1px solid #ccc; | |
| `; | |
| const TooltipTitle = styled.p` | |
| font-weight: bold; | |
| margin-bottom: 5px; | |
| `; | |
| const TooltipProbability = styled.p` | |
| color: #8884d8; | |
| `; | |
| const TooltipSubjective = styled.p` | |
| font-style: italic; | |
| margin-top: 5px; | |
| font-size: 0.9em; | |
| color: #555; | |
| `; | |
| // Custom Tooltip for enhanced details | |
| const CustomTooltip = ({ active, payload, label }: any) => { | |
| if (active && payload && payload.length) { | |
| const data = payload[0].payload; | |
| return ( | |
| <TooltipContainer> | |
| <TooltipTitle> | |
| {label ? new Date(label).toUTCString() : ''} | |
| </TooltipTitle> | |
| <TooltipProbability>Probability: {data.probability}</TooltipProbability> | |
| <p>Event: {data.event}</p> | |
| <p>Outcome: {data.outcome}</p> | |
| <TooltipSubjective> | |
| "{data.subjective}" | |
| </TooltipSubjective> | |
| </TooltipContainer> |
| export function getProbabilityData() { | ||
| return telemetryData.map(entry => ({ | ||
| time: new Date(entry.utc).getTime(), // For x-axis | ||
| probability: entry.prediction.p, | ||
| event: entry.event, | ||
| outcome: entry.outcome, | ||
| subjective: entry.subjective, | ||
| })); | ||
| } |
There was a problem hiding this comment.
Consider creating a TypeScript interface for the return type of getProbabilityData() to ensure type safety throughout the component chain. This would help catch potential issues if the data structure changes in the future.
| if (active && payload && payload.length) { | ||
| const data = payload[0].payload; | ||
| return ( | ||
| <div className="card" style={{ padding: '10px', border: '1px solid #ccc' }}> |
There was a problem hiding this comment.
The tooltip div uses className="card" but this class may not provide the expected styling. Consider using a more specific class name or removing this className if the inline styles are sufficient, to avoid confusion.
| <div className="card" style={{ padding: '10px', border: '1px solid #ccc' }}> | |
| <div style={{ padding: '10px', border: '1px solid #ccc' }}> |
| // Custom Tooltip for enhanced details | ||
| const CustomTooltip = ({ active, payload, label }: any) => { |
There was a problem hiding this comment.
The CustomTooltip component uses 'any' type for its props, which bypasses TypeScript's type safety. Consider creating a proper interface for the tooltip props that includes the structure of active, payload, and label parameters.
| // Custom Tooltip for enhanced details | |
| const CustomTooltip = ({ active, payload, label }: any) => { | |
| interface CustomTooltipPayload { | |
| probability: number; | |
| event: string; | |
| outcome: string; | |
| subjective: string; | |
| } | |
| interface CustomTooltipProps { | |
| active?: boolean; | |
| label?: string | number; | |
| payload?: Array<{ | |
| payload: CustomTooltipPayload; | |
| }>; | |
| } | |
| // Custom Tooltip for enhanced details | |
| const CustomTooltip: React.FC<CustomTooltipProps> = ({ active, payload, label }) => { |
1 participant
💡 What: Added a custom tooltip to the Telemetry Timeline chart to display detailed event info (Event, Outcome, Subjective) and improved the data table accessibility by adding scope attributes and a caption.
🎯 Why: Users needed to cross-reference the table to understand chart spikes. Screen reader users lacked context in the data table.
♿ Accessibility: Added scope="col" to table headers and a descriptive caption.
See .Jules/palette.md for design journal.
PR created automatically by Jules for task 8764651375751760842 started by @igor-holt