fix: SW-1889 stop chart title defaulting to component name#152
Open
owilliams-tetrascience wants to merge 1 commit into
Open
fix: SW-1889 stop chart title defaulting to component name#152owilliams-tetrascience wants to merge 1 commit into
owilliams-tetrascience wants to merge 1 commit into
Conversation
owilliams-tetrascience
temporarily deployed
to
artifactory-prod
GitHub Actions
Inactive
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
owilliams-tetrascience
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates several chart components to avoid rendering an unintended “placeholder” title (and associated top whitespace) when consumers omit the title prop, and adds Storybook coverage for the no-title behavior.
Changes:
- Removed the default
titlevalue (component name) forAreaGraph,BarGraph,LineGraph, andPieChart. - For Plotly-based charts (
AreaGraph,BarGraph,LineGraph), conditionally omitlayout.titleand reducelayout.margin.twhentitleis unset via extracted constants. - Added “No Title” Storybook stories (with play-function assertions) to ensure the title block is absent while charts still render.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/charts/AreaGraph/AreaGraph.tsx | Makes title optional, omits Plotly title when unset, and reduces top margin when there’s no title. |
| src/components/charts/AreaGraph/AreaGraph.stories.tsx | Adds a “No Title” story asserting the Plotly title group is not rendered. |
| src/components/charts/BarGraph/BarGraph.tsx | Makes title optional, conditionally includes Plotly title, and adjusts margin.t based on title presence. |
| src/components/charts/BarGraph/BarGraph.stories.tsx | Adds a “No Title” story asserting no Plotly title is rendered and the chart still draws. |
| src/components/charts/LineGraph/LineGraph.tsx | Makes title optional, conditionally includes Plotly title, and adjusts margin.t based on title presence. |
| src/components/charts/LineGraph/LineGraph.stories.tsx | Updates stories to set title explicitly where needed and adds a “No Title” story. |
| src/components/charts/PieChart/PieChart.tsx | Removes the default title so the JSX title block won’t render unless a title is provided. |
| src/components/charts/PieChart/PieChart.stories.tsx | Adds a “No Title” story asserting the PieChart title elements are absent. |
Comments suppressed due to low confidence (1)
src/components/charts/PieChart/PieChart.tsx:46
- PR description mentions shrinking the Plotly top margin when
titleis unset so no top whitespace is reserved, butPieChartstill hard-codeslayout.margin.tto40regardless oftitle. If the whitespace issue applies toPieCharttoo, this will leave a fixed 40px gap even whentitleis omitted.
const PieChart: React.FC<PieChartProps> = ({
dataSeries,
width = 400,
height = 400,
title,
textInfo = "percent",
hole = 0,
rotation = 0,
}) => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
54321jenn-ts
approved these changes
Jun 23, 2026
AreaGraph, BarGraph, LineGraph, and PieChart hard-defaulted their
`title` prop to the literal component name ("Area Graph", etc.), so any
consumer that omitted `title` got a 32px placeholder title plus reserved
top margin leaking into production UIs.
- Default `title` to `undefined` in all four charts.
- Omit the Plotly title block (and the JSX title in PieChart) when no
title is set, and shrink `margin.t` accordingly. Top margins extracted
to TITLE_MARGIN_TOP / NO_TITLE_MARGIN_TOP constants.
- Add a "No Title" Storybook story per chart asserting no title block
renders while the chart still draws.
- Six existing LineGraph stories relied on the old default; set their
title explicitly so they keep asserting "Line Graph".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
owilliams-tetrascience
force-pushed
the
claude/bold-buck-cb30f3
branch
from
4238196 to
8a4d7f0
Compare
owilliams-tetrascience
temporarily deployed
to
artifactory-prod
GitHub Actions
Inactive
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Fixes SW-1889.
AreaGraph,BarGraph,LineGraph, andPieCharthard-defaulted theirtitleprop to the literal component name ("Area Graph","Bar Graph","Line Graph","Pie Chart"). Any consumer that omittedtitlegot a 32px placeholder title rendered in production, plus ~60px of reserved top margin — especially destructive on small dashboard tiles.Changes
titletoundefinedin all four chart components.titleis unset, omit the title block entirely — the Plotlylayout.titleis no longer included (and PieChart's JSX<h2>was already guarded) — and shrinkmargin.tso no top whitespace is reserved. The two top-margin values are extracted to namedTITLE_MARGIN_TOP/NO_TITLE_MARGIN_TOPconstants (required to satisfyno-magic-numbers, which flags ternary operands)..gtitlefor Plotly charts,.titlefor PieChart) while the chart still renders.LineGraphstories relied on the old default and asserted"Line Graph"was present — they now settitle: "Line Graph"explicitly so they keep passing (per the acceptance criteria).Testing
yarn typecheck✅yarn lint✅ (0 warnings)yarn test✅ (548 unit tests)yarn test:storybook✅ (657 play-function tests)Reviewer notes
parameters.zephyr.testCaseIdas""— apply thezephyr_synclabel so the workflow generates and commits the real IDs.