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] Fix some props not working in xAxis/yAxis #13372

Merged
merged 10 commits into from
Jun 5, 2024

Conversation

Valyok26
Copy link
Contributor

@Valyok26 Valyok26 commented Jun 4, 2024

Fixes #13371

When props are assigned, priority is bottomAxis -> xAxis -> theme -> defaults.

Also while fixing this, found out that tickNumber and reverse cannot be assigned with bottomAxis, but that's much harder to fix.
Also position cannot be set with xAxis/yAxis, but I'm not sure it worth adding lines to ChartsAxis.tsx just to change this behavior.

@mui-bot
Copy link

mui-bot commented Jun 4, 2024

Deploy preview: https://deploy-preview-13372--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against f2bbc8d

@michelengelen michelengelen added bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module! labels Jun 5, 2024
@alexfauquette
Copy link
Member

@JCQuintas COuld you review the last commits?

The PR introduces following line, I assume to fix TS

position: inProps.position || defaultProps.position,

The TS issue came from the settings which has type AxisConfig, including attributes position: 'top' | 'bottom' | 'left' | 'right'.

This last commit makes sure that x-axis settings only get the x-axis attribute (top and bottom) and the y-axis only get the left and right.

@JCQuintas
Copy link
Member

@alexfauquette looked a bit at @Valyok26 comments that tickNumber and reverse doesn't work. It seems that for the bottom axis that is expected, since it is a type:"band".

Though CartesianContextProvider doesn't take theme into account, even for the Y axis, it doesn't work unless you pass it as a prop. Not sure if it is intentional or we should create an issue. 🤔

@alexfauquette
Copy link
Member

For reverse it's expected. It's not something that only impacts the axis rendering. It impacts the entire rendering of the chart so it can only be set in axis config.

For the tickNumber it's more surprising. I will have a second look

Copy link
Member

@JCQuintas JCQuintas left a comment

Choose a reason for hiding this comment

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

Seems to be working as expected. We will look at the ticksNumber in another issue. Thanks @Valyok26 for the PR 😃

@JCQuintas JCQuintas merged commit 48d1269 into mui:master Jun 5, 2024
17 checks passed
@alexfauquette
Copy link
Member

ticksNumber has the same issue. it's less obvious, but tick number is used when generating the d3 scale. So it's used during the axis processing, not in the axis rendering.

@JCQuintas
Copy link
Member

It's unclear what would be our next steps, should we remove the ticksNumber type from theme overrides, or should we make sure we are able to load it from theme?

@JCQuintas
Copy link
Member

If I think about it, it would probably make sense that the CartesianContextProvider takes theme props into account, since props are configuration as well, although it could make things a lot more complex 🙃

DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024
Co-authored-by: Jose Quintas <juniorquintas@gmail.com>
Co-authored-by: alexandre <alex.fauquette@gmail.com>
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Co-authored-by: Jose Quintas <juniorquintas@gmail.com>
Co-authored-by: alexandre <alex.fauquette@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work 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.

[charts] Some axis props cannot be assigned with xAxis/yAxis
5 participants