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

Fix Flyout scrolling in Safari #2033

Merged
merged 3 commits into from
Jun 13, 2019
Merged

Conversation

Kerry350
Copy link
Contributor

Summary

This issue was originally picked up in the Logs UI. The changes here allow the body of the Flyout component to scroll in Safari, currently in master it won't scroll at all. My understanding is that, according to the Flebox spec, for height to take effect it needs to be present throughout the whole Flex hierarchy (so Safari was doing the right thing 😭).

However, these changes do seem to introduce the classic body scrolling issue, which on master currently only exists in IE11. By body scrolling issue I mean that when the scrollable area of the flyout is exhausted the body will start to scroll. In the past I've generally solved this with a noscroll class on the body when the component is open, or hovered (depending on the scenario). Do EUI have a standard way of handling this?

(Aside: there will also be a Kibana PR to update the legacy style for .header-global-wrapper + .app-wrapper:not(.hidden-chrome) .euiFlyout, I'll open that once this PR is merged.).

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

@Kerry350 Kerry350 requested a review from a team June 12, 2019 10:36
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks @Kerry350 !! This thing has been a big PIA since Chrome introduced a flex overflow bug.

I checked this in:

  • Mac: Chrome, Safari, Firefox
  • Win 7: IE11, Chrome

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
@Kerry350 Kerry350 merged commit 1479c81 into elastic:master Jun 13, 2019
@Kerry350
Copy link
Contributor Author

@cchaos Thanks for the review 🤘

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

Successfully merging this pull request may close these issues.

2 participants