[charts] Use <circle /> instead of <path /> for line marks by default
#15220
+231
−83
Conversation
This file contains 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
oliviertassinari
added
the
component: charts
CodSpeed Performance ReportMerging #15220 will not alter performanceComparing Summary
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
JCQuintas
reviewed
Jan 9, 2025
| Notice that using another shape than "circle" will render a `<path />` instead of the `<circle />` for mark elements. | ||
| This modification implies a small drop of rendering performances (around +50ms to render 1.000 marks). | ||
|
|
||
| {{"demo": "CustomeLineMarks.js"}} |
JCQuintas
reviewed
Jan 9, 2025
|
|
||
| ## CustomeLineMarks | ||
|
|
||
| Notice that using another shape than "circle" will render a `<path />` instead of the `<circle />` for mark elements. |
There was a problem hiding this comment.
Suggested change
| Notice that using another shape than "circle" will render a `<path />` instead of the `<circle />` for mark elements. | |
| Notice that using another shape than "circle" renders a `<path />` instead of the `<circle />` for mark elements. |
JCQuintas
reviewed
Jan 9, 2025
Comment on lines
+153
to
+158
| The `experimentalMarkRendering` prop has been removed from the LineChart component. | ||
| The line mark are now `<circle />` element by default. | ||
| And you can chose another shape by adding a `shape` property to your line series. | ||
|
|
||
| The codemod only removes the `experimentalMarkRendering` prop. | ||
| If you relied on the fact that marks were `path` elements, you need to update your logic. |
There was a problem hiding this comment.
Probably should add this to the changelog in PR description
JCQuintas
reviewed
Jan 9, 2025
| @@ -85,7 +85,7 @@ describe('LineChart - click event', () => { | |||
| onMarkClick={() => {}} | |||
| />, | |||
| ); | |||
| const marks = document.querySelectorAll<HTMLElement>('path.MuiMarkElement-root'); | |||
| const marks = document.querySelectorAll<HTMLElement>('circle.MuiMarkElement-root'); | |||
There was a problem hiding this comment.
Cant we target only .MuiMarkElement-root?
JCQuintas
reviewed
Jan 9, 2025
Comment on lines
75
to
80
| /** | ||
| * The shape of the mark elements. | ||
| * Notice that using something else than 'circle' will render <path /> instead of <circle /> leading to a small performance decrease. | ||
| * @default 'circle' | ||
| */ | ||
| shape?: 'circle' | 'cross' | 'diamond' | 'square' | 'star' | 'triangle' | 'wye'; |
There was a problem hiding this comment.
Suggested change
| /** | |
| * The shape of the mark elements. | |
| * Notice that using something else than 'circle' will render <path /> instead of <circle /> leading to a small performance decrease. | |
| * @default 'circle' | |
| */ | |
| shape?: 'circle' | 'cross' | 'diamond' | 'square' | 'star' | 'triangle' | 'wye'; | |
| /** | |
| * The shape of the mark elements. | |
| * Using 'circle' renders a `<circle />` element, while all other options render a `<path />` instead. The path causes a small decrease in performance. | |
| * @default 'circle' | |
| */ | |
| shape?: 'circle' | 'cross' | 'diamond' | 'square' | 'star' | 'triangle' | 'wye'; |
There was a problem hiding this comment.
Original felt a bit odd starting with a negative
JCQuintas
approved these changes
Jan 9, 2025
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.
Previously the notion of shape was not working for line mark. It was always rendering a
pathwithd={d3.circle()}Now the shape is working, and if it's a circle we render a
circleotherwise apathwith the appropriate d3 shape.This modification is a breaking change because it modifies the SVG component which for example break our tests on mark.click which expect an, event coming from the
pathChangelog
Chart breakingchange
The
experimentalMarkRenderingprop has been removed from the LineChart component.The line mark are now
<circle />element by default.And you can chose another shape by adding a
shapeproperty to your line series.The codemod only removes the
experimentalMarkRenderingprop.If you relied on the fact that marks were
pathelements, you need to update your logic.