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.

Sign up for GitHub

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

Jump to bottom

[charts] Allow onItemClick on the Legend component #14231

Merged
merged 30 commits into from
Sep 11, 2024
Merged

[charts] Allow onItemClick on the Legend component #14231

merged 30 commits into from
Sep 11, 2024

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented Aug 16, 2024 •
edited
Loading

  • Works on ChartsLegend and PiecewiseColorLegend

  • Allows simple customisation of behavior, not sure it is worth documenting 🤔

  • Should we try to provide this for ContinuousColorLegend? We could try to extrapolate the current color or value the user is clicking on and pass it to the user 🤔

Would be a middleground quick-win for #12344, though no actions yet

@JCQuintas JCQuintas added enhancement This is not a bug, nor a new feature component: charts This is the name of the generic UI component, not the React module! labels Aug 16, 2024
@JCQuintas JCQuintas self-assigned this Aug 16, 2024
@mui-bot
Copy link

mui-bot commented Aug 16, 2024 •
edited
Loading

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

Updated pages:

Generated by 🚫 dangerJS against 9268123

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Aug 16, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #14231 will not alter performance

Comparing JCQuintas:12344-charts-legend-actions-eg-highlighting (9268123) with master (4f2a244)

Summary

✅ 3 untouched benchmarks

@JCQuintas JCQuintas changed the title [charts] Allow onClick on the Legend component [charts] Allow onItemClick on the Legend component Aug 16, 2024
@alexfauquette
Copy link
Member

Should we try to provide this for ContinuousColorLegend? We could try to extrapolate the current color or value the user is clicking on and pass it to the user 🤔

I'm wondering if we add just the onItem click are we shooting in our foot for a more interactive ContinuousColorLegend. It might be complicated to add range handlers if we already have a onItemClick.

Do you have a specific uscase in mind that would be possible to create with this onItemClick for continuous range?

@JCQuintas
Copy link
Member Author

Do you have a specific uscase in mind that would be possible to create with this onItemClick for continuous range?

Not really, it was just a thought that occurred to me when I noticed the continuous label is very different from the others.

I suspect behaviour could be derived for things like, click on 1.54 on the continuous scale the user could filter whatever they like (values between 0.54 to 2.54, but I left it as a comment mostly because I don't see much value in that 😅

@alexfauquette
Copy link
Member

Not really, it was just a thought that occurred to me when I noticed the continuous label is very different from the others.

Ok, then waiting for an explicit request from a user might be the best approach

@github-actions GitHub Actions
Copy link

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 Aug 27, 2024
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Aug 27, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 27, 2024
@github-actions GitHub Actions
Copy link

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

@JCQuintas
Copy link
Member Author

Waiting for resolution of #14391 and #14392 before finalising this

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 30, 2024
@github-actions 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 Sep 2, 2024
@JCQuintas JCQuintas marked this pull request as ready for review September 2, 2024 17:46
alexfauquette
alexfauquette reviewed Sep 4, 2024
View reviewed changes
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Looks nice. Just left minor comments

docs/data/charts/legend/legend.md Outdated Show resolved Hide resolved
docs/data/charts/legend/legend.md Outdated Show resolved Hide resolved
docs/data/charts/legend/legend.md Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/ChartsLegend.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/ChartsLegendItem.tsx Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/ChartsLegendItem.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/chartsLegend.types.ts Outdated
Comment on lines 39 to 42
/**
* The identifier of the pie item
*/
itemId: PieItemId;
Copy link
Member

Choose a reason for hiding this comment

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

Not exactly since all the series provide both. For Bar, Line, and Scatter those itemId will be the same as seriesId

Copy link
Member Author

Choose a reason for hiding this comment

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

True, though I felt it unnecessary to mention The identifier of the pie item, uses the seriesId in other types

Initially I had type: pie as a different shape, though I felt it to be a bit cumbersome and moved itemId to the type: series 😅

Copy link
Member

Choose a reason for hiding this comment

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

And what about itemId?: PieItemId; with itemId not provided is the series is not a pie?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦

packages/x-charts/src/ChartsLegend/chartsLegend.types.ts Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/chartsLegend.types.ts Outdated Show resolved Hide resolved
alexfauquette
alexfauquette approved these changes Sep 11, 2024
View reviewed changes
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

👍

@JCQuintas JCQuintas enabled auto-merge (squash) September 11, 2024 19:16
@JCQuintas JCQuintas merged commit b0703ab into mui:master Sep 11, 2024
17 checks passed
@JCQuintas JCQuintas deleted the 12344-charts-legend-actions-eg-highlighting branch September 11, 2024 19:32
arthurbalduini pushed a commit to arthurbalduini/mui-x that referenced this pull request Sep 30, 2024
@JCQuintas @alexfauquette
[charts] Allow onItemClick on the Legend component (mui#14231)
6c2befb 
Signed-off-by: Jose C Quintas Jr <[email protected]>
Co-authored-by: Alexandre Fauquette <[email protected]>
@mbreadmachine mbreadmachine mentioned this pull request Oct 5, 2024
Closed
This was referenced Oct 5, 2024
Open
Open
Open
@mbreadmachine mbreadmachine mentioned this pull request Oct 6, 2024
Closed
This was referenced Oct 25, 2024
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexfauquette alexfauquette alexfauquette approved these changes

Assignees

@JCQuintas JCQuintas

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 size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JCQuintas @mui-bot @alexfauquette