Skip to content

feat(legend): add custom legend component#1889

Merged
markov00 merged 7 commits intoelastic:mainfrom
yannbolliger:custom-legend
Dec 19, 2022
Merged

feat(legend): add custom legend component#1889
markov00 merged 7 commits intoelastic:mainfrom
yannbolliger:custom-legend

Conversation

@yannbolliger
Copy link
Copy Markdown
Contributor

@yannbolliger yannbolliger commented Dec 2, 2022 •
edited
Loading

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
Copy Markdown

cla-checker-service bot commented Dec 2, 2022 •
edited
Loading

💚 CLA has been signed

@elastic-datavis
Copy link
Copy Markdown
Contributor

elastic-datavis bot commented Dec 2, 2022

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

@nickofthyme
Copy link
Copy Markdown
Collaborator

nickofthyme commented Dec 2, 2022

buildkite test this

markov00
markov00 reviewed Dec 2, 2022
View reviewed changes
@elastic-datavis
Copy link
Copy Markdown
Contributor

elastic-datavis bot commented Dec 5, 2022

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

@elastic-datavis
Copy link
Copy Markdown
Contributor

elastic-datavis bot commented Dec 8, 2022

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

@yannbolliger yannbolliger marked this pull request as ready for review December 8, 2022 09:40
markov00
markov00 reviewed Dec 12, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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

@yannbolliger
Copy link
Copy Markdown
Contributor Author

yannbolliger commented Dec 12, 2022

buildkite update screenshots

@markov00
Copy link
Copy Markdown
Collaborator

markov00 commented Dec 12, 2022

buildkite test this

@nickofthyme
Copy link
Copy Markdown
Collaborator

nickofthyme commented Dec 12, 2022

buildkite update screenshots

@nickofthyme nickofthyme mentioned this pull request Dec 12, 2022
Closed
@nickofthyme
Copy link
Copy Markdown
Collaborator

nickofthyme commented Dec 12, 2022

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
Copy Markdown
Collaborator

nickofthyme commented Dec 12, 2022

buildkite test this

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

nickofthyme commented Dec 14, 2022

buildkite test this

@markov00
Copy link
Copy Markdown
Collaborator

markov00 commented Dec 14, 2022

@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
Copy Markdown
Contributor Author

yannbolliger commented Dec 14, 2022

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
Copy Markdown
Collaborator

markov00 commented Dec 14, 2022

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
Copy Markdown
Contributor Author

yannbolliger commented Dec 19, 2022 •
edited
Loading

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.)

markov00
markov00 approved these changes Dec 19, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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
@semantic-release-bot
chore(release): 51.2.0 [skip ci]
b335336 
# [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

Reviewers

@markov00 markov00 markov00 approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request :legend Legend related issue

Projects

None yet

Milestone

No milestone

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

@yannbolliger @nickofthyme @markov00