[charts] Fix docs/translations/errors typos #12941
Conversation
JCQuintas
added
enhancement
|
Deploy preview: https://deploy-preview-12941--material-ui-x.netlify.app/ |
| @@ -8,7 +8,7 @@ import ChartsContinuousGradient from './ChartsContinuousGradient'; | |||
| export function useChartGradient() { | |||
| const { chartId } = React.useContext(DrawingContext); | |||
| return React.useCallback( | |||
| (axisId: string, direction: 'x' | 'y') => `${chartId}-graient-${direction}-${axisId}`, | |||
| (axisId: string, direction: 'x' | 'y') => `${chartId}-gradient-${direction}-${axisId}`, | |||
There was a problem hiding this comment.
This is technically a BC, albeit, so small that, IMHO, we could go ahead with it...
Did you think about it? What do you think? 🤔
There was a problem hiding this comment.
Hum, I didn't look much into because no docs changed due to this.
Upon further understanding what it does, it doesn't look like it is documented for external use nor it affects any of the tests.
Do you have more concrete examples of what issues this change could create?
There was a problem hiding this comment.
It's only the fact that an id DOM attribute changes and someone could be relying on it, hence, essentially it is a breaking change.
However, as you have also pointed out, it is not documented and essentially nothing changes functionality wise, that's why I mentioned that, IMHO, it is fine to go ahead with this change.
I just asked if you have considered it. 👍
If it were any more important, we could add a breaking change note in the release changelog, but I'm not sure if it is even worth that. 🙈 😆
There was a problem hiding this comment.
Do you think we should put a small note in the changelog about it? "Fixed a typo in an undocumented x-charts gradient css class that could have unintended sideeffects"?
There was a problem hiding this comment.
It won't hurt. :)
However, am I missing something or does it end up in the id field instead of a class? 🤔
| @@ -8,7 +8,7 @@ import ChartsContinuousGradient from './ChartsContinuousGradient'; | |||
| export function useChartGradient() { | |||
| const { chartId } = React.useContext(DrawingContext); | |||
| return React.useCallback( | |||
| (axisId: string, direction: 'x' | 'y') => `${chartId}-graient-${direction}-${axisId}`, | |||
| (axisId: string, direction: 'x' | 'y') => `${chartId}-gradient-${direction}-${axisId}`, | |||
There was a problem hiding this comment.
It's only the fact that an id DOM attribute changes and someone could be relying on it, hence, essentially it is a breaking change.
However, as you have also pointed out, it is not documented and essentially nothing changes functionality wise, that's why I mentioned that, IMHO, it is fine to go ahead with this change.
I just asked if you have considered it. 👍
If it were any more important, we could add a breaking change note in the release changelog, but I'm not sure if it is even worth that. 🙈 😆
Didn't consider, but should be pretty simple to run in CI, and yes it is fast. I just think it needs to be a bit more thoughtful process. Because it flags a lot of "fake/tech" words as "wrong". Eg:
I believe you can do that with anything using some shell scripting magic 😛 |
I just ran cspell on the x-charts folder and fixed what didn't look like intentional 😅