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

[EuiTour] Fix positioning calculation issues caused by width style overrides #5897

Merged
merged 6 commits into from
May 17, 2022

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented May 12, 2022

Summary

closes #5861
closes #5731 (I'm relatively confident this is the same root problem)

QA can be performed on the Data Grid example (I was trying to recreate/repro the issue screencapped by a separate Kibana setup)

Before

before

After

after

Why was this bug happening?

Unfortunately, EuiPopover's popover positioning calculation does not elegantly handle overriding the width of the popover via either CSS or styling of width, max-width, or min-width on the popover panel itself. Instead, the "correct" (or non-bug-inducing-way) to manage a popover's width is to restrict it with a wrapper around the popover's contents (which is the change that this PR made).

I could dive more deeply into EuiPopover's popover_positioning service to try and figure out how to work around this, but that seemed like a much more involved fix compared to this much simpler one, and since it hasn't come up before except for EuiTour, I figured there wasn't a super pressing need for it.

Checklist

  • Checked in Chrome, Safari, Edge, and Firefox
  • Added or updated jest and cypress tests
  • A changelog entry exists and is marked appropriately

@cee-chen cee-chen requested a review from thompsongl May 12, 2022 20:00
@cee-chen
Copy link
Member Author

cee-chen commented May 12, 2022

I built this PR and bootstrapped it in Kibana to test and can confirm the issue @afgomez reported in #5861 is fixed :) (EDIT: Apologies, autocomplete tagged the wrong user!)

fix1

Unfortunately I'm a huge o11y noob and can't figure out how to get data into APM to test the screen that @cauemarcondes reported in #5731 🙈 I'm like 90% sure this PR will still fix that issue but can't 100% confirm, unless someone wants to help me out with setting up my local env to get to that screen

@cee-chen
Copy link
Member Author

cee-chen commented May 12, 2022

Thanks so much for helping me get set up @cauemarcondes! I can confirm the issue reported in #5731 should be fixed, and the EuiTour will be properly left aligned in Kibana once this lands.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5897/

…t` and doesn't seem necessary to explicitly document"

This reverts commit e121e98.
@cee-chen cee-chen requested a review from thompsongl May 17, 2022 15:47
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5897/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM! Nice find

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants