Skip to content
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

[charts] Add <ChartsReferenceLine /> component #10597

Merged
merged 28 commits into from
Nov 8, 2023

Conversation

wascou
Copy link
Contributor

@wascou wascou commented Oct 7, 2023

Referencing #10351

@alexfauquette alexfauquette self-requested a review October 9, 2023 10:05
@alexfauquette
Copy link
Member

I've updated the PR to use x/y instead of threshold/direction.

I've kind of removed some features of the <text /> since they should be available when #10138 got merged

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Oct 9, 2023
@mui-bot
Copy link

mui-bot commented Oct 9, 2023

@alexfauquette alexfauquette changed the title ChartsReferenceLine [charts] Add <ChartsReferenceLine /> component Oct 10, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 11, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@alexfauquette
Copy link
Member

@wascou don't worry about CI, I can handle it

The #10138 just got merged so it will be possible to add <Text/> to easily manage multi-line text in the reference line.

After that, I think we will be good for merging. Maybe the documentation could get a bit more attention

@wascou
Copy link
Contributor Author

wascou commented Oct 13, 2023

I can give few examples?

@alexfauquette
Copy link
Member

alexfauquette commented Oct 13, 2023

I can give few examples?

With pleasure. For consistency, I usually put two examples:

But for now, I would recommend to wait a bit. I'm working on adding the #10138 and its followup wihich might implies a bit more modification 🫣

@wascou
Copy link
Contributor Author

wascou commented Oct 13, 2023

Good.
And as you changed props structure, now, my own example does not fit:

<ReferenceLine
                direction="horizontal"
                threshold={100}
                color="red"
                lineWidth={1}
                text="Limit"
              />

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 13, 2023
@alexfauquette
Copy link
Member

Here I think I have reached something stable.

I think it's able to the the same stuff as the initial proposal but with fewer props.

<ReferenceLine
  label="Example\nwith multi line" // the label to display
  x={10} // the reference value
  labelAlign="start" // To place the label on the start, middle, or end of the reference line
  spacing={10} // the space between the line and the text
  lineStyle={{ strokeDasharray: '10 5' }} // Every style customization of the line in a single object
  labelStyle={{ strokeColor: 'red' }} // Every style customization of the label in a single object
/>

Your example would become

 <ReferenceLine
-  direction="horizontal"
-  threshold={100}
+  y={100}
-  color="red"
-  lineWidth={1}
+  lineStyle={{ strokeWidth: 1, strokeColor: 'red' }}
-   text="Limit"
+   label="Limit"
/>

I think the was textAnchor and dominantBaseline are passed to <ChartsText/> might change again. But that's more an internal consideration

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 19, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 24, 2023
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice addition! 💯
Great job! 👍
Leaving some comments for possible improvements and one functional fix that IMHO we should do. 🤔

} = props;

const { top, height } = useDrawingArea();
const xAxisScale = useXScale(axisId) as any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Was a more proper solution considered, which would avoid the need for as any? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been more complicated than expected.

Each chart has some expectations about which scale they use, but useXScale returns any type of scale.

So the next line xAxisScale(x) was failing because x could be for example a string (available if using band scale) but the axis scale returned by xAxisScale could be linear which only accepts number

On the way of experimenting, I structured a bit more the scale types to remove as much any as possible in scales type def

docs/data/charts/axis/axis.md Outdated Show resolved Hide resolved
docs/data/charts/axis/axis.md Outdated Show resolved Hide resolved
docs/data/charts/axis/axis.md Show resolved Hide resolved

case 'end':
return {
y: top + height,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, we should solve this problem:
Screenshot 2023-11-01 at 13 01 00
At least changing the dominantBaseline to ideographic avoids the label overlapping the ChartsXAxis.
Or maybe it would also make sense to introduce or change the spacing prop to accept the x and y prop so that the end user could tailor the result to their needs?

Co-authored-by: Lukas <[email protected]>
Signed-off-by: Alexandre Fauquette <[email protected]>
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 2, 2023
Copy link

github-actions bot commented Nov 2, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@michelengelen michelengelen changed the base branch from master to next November 6, 2023 14:21
@alexfauquette alexfauquette changed the base branch from next to master November 7, 2023 12:24
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 7, 2023
@alexfauquette alexfauquette requested a review from LukasTy November 7, 2023 14:18
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a really well executed addition. Kudos to everyone involved! 👏 💯

@michelengelen
Copy link
Member

@alexfauquette can we switch the base branch to next and possibly backport these changes to master?

@alexfauquette alexfauquette merged commit efa968b into mui:master Nov 8, 2023
5 checks passed
alexfauquette added a commit to alexfauquette/mui-x that referenced this pull request Nov 8, 2023
Co-authored-by: alexandre <[email protected]>
Co-authored-by: Alexandre Fauquette <[email protected]>
Co-authored-by: Lukas <[email protected]>
alexfauquette added a commit that referenced this pull request Nov 8, 2023
@wascou wascou deleted the ChartReferenceLine branch November 8, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants