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

[Visualize] Coloring tooltips in Pie are not properly positioned #124330

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Feb 2, 2022

Closes: #116748

Summary

Steps to reproduce:

  1. Create a pie chart with a visible legend.
  2. Set up a legend to be on the top or bottom.
  3. try to change a color by clicking on a color indicator

Screen:

Screen.Recording.2022-02-02.at.12.44.51.PM.mov

What was changed:

  • EuiPopover -> EuiWrappingPopover
  • Created a LegendColorPickerWrapperContext context. This context is used instead of useMemo patterrn
  • useMemo for getColorPicker was removed;

Related to elastic/eui#4367

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
expressionPie 39.3KB 39.5KB +193.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@alexwizp alexwizp marked this pull request as ready for review February 2, 2022 12:05
@alexwizp alexwizp requested a review from a team as a code owner February 2, 2022 12:05
@alexwizp alexwizp self-assigned this Feb 2, 2022
@alexwizp alexwizp added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v8.1.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Feb 2, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@mbondyra
Copy link
Contributor

mbondyra commented Feb 2, 2022

Hey @alexwizp what was causing the issue? Trying to spot it in the code but it's unclear to me.

Is this one outdated and can be closed? #123042

@stratoula
Copy link
Contributor

Yes @mbondyra! Diana's PR can close in favor of this one. I am closing this

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Tested on Chrome and Safari and works fine! I am just curious about the initial cause :)

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

I also tested that and works fine! 🎉 Code LGTM

@nickofthyme
Copy link
Contributor

This looks great @alexwizp. I slightly recall there being an issue using one or the other (EuiWrappingPopover vs EuiPopover) with keyboard accessibility and returning focus to the correct element. But if the focus works fine I'm fine with the change. @rshen91 do you recall what this issue was?

@rshen91
Copy link
Contributor

rshen91 commented Feb 2, 2022

Also am fine with this change thanks @nickofthyme. The EuiPopover should better handle the focus management of the color icon vs sending it back to window. Thank you for implementing this 💪🏻

@alexwizp alexwizp merged commit 5436013 into elastic:main Feb 3, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Visualize] Coloring tooltips in Pie are not properly positioned
8 participants