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

feat(legend): add custom legend component #1889

Merged
merged 7 commits into from
Dec 19, 2022

Conversation

yannbolliger
Copy link
Contributor

@yannbolliger yannbolliger commented Dec 2, 2022

Summary

The settings now have a new prop customLegend which lets users provide there own legend React component that is rendered with the chart state into the legend container.

Details

Analogously to the customTooltip in the Tooltip, the customLegend takes a React component which will receive the state of the standard legend as props. This allows users to provide their own legend implementation. The legend is still rendered inside of the div.echLegendListContainer.

Issues

Fixes #862 because it is now feasible with a custom component as well as fixes #561.

Checklist

  • All related issues have been linked (i.e. closes #123, fixes #123)
  • The proper documentation and/or storybook story has been added or updated
  • Unit tests have been added or updated to match the most common scenarios
  • New public API exports have been added to packages/charts/src/index.ts
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)

@cla-checker-service
Copy link

cla-checker-service bot commented Dec 2, 2022

💚 CLA has been signed

@elastic-datavis
Copy link
Contributor

Community pull request, @elastic/datavis please add the ci:approved ✅ label to allow this and future builds.

@nickofthyme
Copy link
Collaborator

buildkite test this

@elastic-datavis
Copy link
Contributor

Community pull request, @elastic/datavis please add the ci:approved ✅ label to allow this and future builds.

@elastic-datavis
Copy link
Contributor

Community pull request, @elastic/datavis please add the ci:approved ✅ label to allow this and future builds.

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

I've took the chance to push a commit to:

  • fix the legend style (avoiding the box shadow on the legend title)
  • add a small notice about always using the legendSize when using the customLegend (it can probably be enforced with typescript somehow, because if not the auto-legend-size will kick in and the available size for the legend will not reflect the actual custom legend size

packages/charts/src/specs/settings.tsx Outdated Show resolved Hide resolved
@yannbolliger
Copy link
Contributor Author

buildkite update screenshots

@markov00
Copy link
Member

buildkite test this

@nickofthyme
Copy link
Collaborator

buildkite update screenshots

@nickofthyme
Copy link
Collaborator

Sorry @yannbolliger the ci should permit you to run the buildkite update screenshots command when the ci:approved ✅ label is added to the pr.

@nickofthyme
Copy link
Collaborator

buildkite test this

@nickofthyme nickofthyme changed the base branch from master to main December 14, 2022 01:04
@nickofthyme
Copy link
Collaborator

buildkite test this

@markov00
Copy link
Member

@nickofthyme @yannbolliger I'm taking a loot at that added selector in the Legend component. It will triggers a react component rerendering (most of the time will not change the DOM but still execute the legend component and diff the results) on every mouse move. I prefer to avoid that if possible. I think we need to improve the current getTooltipInfo to avoid this recomputation as much as possible. I will be back soon with a solution to that

@yannbolliger
Copy link
Contributor Author

that added selector in the Legend component. It will triggers a react component rerendering (most of the time will not change the DOM but still execute the legend component and diff the results) on every mouse move.

We could connect only the custom legend component (and not the regular one) via Redux to this additional selector.

@markov00
Copy link
Member

that added selector in the Legend component. It will triggers a react component rerendering (most of the time will not change the DOM but still execute the legend component and diff the results) on every mouse move.

We could connect only the custom legend component (and not the regular one) via Redux to this additional selector.

That could work, is anyway just a workaround, the output of such selector, is always the same if you hover over the same geometry but you move slightly your mouse

@yannbolliger
Copy link
Contributor Author

yannbolliger commented Dec 19, 2022

OK, so I changed the PR to only provide the pointerValue from Redux to the custom legend component. This should fix what you were concerned about @markov00. (Even though my testing showed that the legend is re-rendered when the cursor moves even on the main branch... but that's probably from the extra values.)

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Tested locally, looks good to me.
Thanks @yannbolliger for the changes.
@nickofthyme Yann was right, we are will rerendering the legend independently from the changes in this PR. The legend selectors are firing too on each mouse move, is better to double-check this and fix it on our selectors.

@markov00 markov00 added enhancement New feature or request :legend Legend related issue labels Dec 19, 2022
@markov00 markov00 changed the title feat(legend): add custom legend component setting feat(legend): add custom legend component Dec 19, 2022
@markov00 markov00 merged commit 2e1648d into elastic:main Dec 19, 2022
nickofthyme pushed a commit that referenced this pull request Dec 19, 2022
# [51.2.0](v51.1.1...v51.2.0) (2022-12-19)

### Bug Fixes

* **axis:** truncate titles longer then the available space ([#1905](#1905)) ([3526a69](3526a69))
* **deps:** update dependency @elastic/eui to ^70.4.0 ([#1893](#1893)) ([6576041](6576041))
* **deps:** update dependency @elastic/eui to v71 ([#1898](#1898)) ([da596a5](da596a5))

### Features

* **legend:** add custom legend component ([#1889](#1889)) ([2e1648d](2e1648d))
@yannbolliger yannbolliger deleted the custom-legend branch December 20, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:approved ✅ Approves PR builds enhancement New feature or request :legend Legend related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow a configurable title of the legend Make the legend extra parameter customizable
3 participants