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 context to axis value formatter #12172

Merged
merged 9 commits into from
Mar 8, 2024

Conversation

alexfauquette
Copy link
Member

Part of #10928

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Feb 22, 2024
* @param {V} value The value to format.
* @returns {string} The string to display.
*/
tickValueFormatter?: (value: V) => string;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hesitating with

  • tickFormatter which is shorter and should not interfere with others
  • valueTickFormatter which has the advantage to group all the formatters at the same place in the alphabetical order

Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting situation. 🤔
tickValueFormatter seems like it makes the most sense, but have you considered going the other way - adding a context to the existing valueFormatter? 🤔

interface ValueFomatterContext {
  location: 'value' | 'tick' | 'tooltip';
  // or
  location?: 'tick' | 'tooltip';
}
///
valueFormatter?: (value: V, context: ValueFomatterContext) => string;

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not had preferences between the two, but effectively it fixes the naming issue 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that is what I like the most about it.
Maybe I do not see some potential problems with it, but it feels that it would also be better suited for cases where we'd need to add the option to format the legend labels or something else as well 🙈

@mui-bot
Copy link

mui-bot commented Feb 22, 2024

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

Updated pages:

Generated by 🚫 dangerJS against f2b64fc

Copy link

github-actions bot commented Mar 7, 2024

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 7, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 7, 2024
@LukasTy LukasTy changed the title [charts] Add specific formatted for axes tick and tooltip axis value [charts] Add context to axis value formatter Mar 8, 2024
@LukasTy LukasTy added the enhancement This is not a bug, nor a new feature label Mar 8, 2024
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.

Great work! 👍 💯
Given the pivot in the solution, I took the liberty to update the PR title.
Feel free to update it if you can think of a more appropriate wording. 👌

Co-authored-by: Lukas <[email protected]>
Signed-off-by: Alexandre Fauquette <[email protected]>
@alexfauquette alexfauquette merged commit b66a472 into mui:next Mar 8, 2024
17 checks passed
@alexfauquette alexfauquette deleted the add-formatters branch March 8, 2024 09:06
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
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! enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants