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

Better overflow shadows and some mobile modal fixes #1829

Merged
merged 9 commits into from
Apr 15, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Apr 12, 2019

Modals

  1. Fixed EuiConfirmationModal responsive sizing

Since most confirmation modals are a simple title, short description and then the buttons, it meant that the buttons (footer) was very far from the intro. I made is so the confirmation style modal is just at the bottom of the screen but that it will still max out at 100vh.

This also fixes a layout issue for title-only confirmation modals:

Normal modals still display in full height:

  1. Using the new euiOverflowShadow mixin instead of separate shadows on header and footer

This is a much simpler and more configurable way to add the overflow shadows, though it does require one extra wrapping div. However, it doesn't require a before element and after element.

The new mixin is also more subtle (most noticeable in the flyout example below).

  1. Allow confirm modal confirm button to be disabled

I can't remember the exact reason this was necessary, but I remember someone asking for it and had made a note of it.

Flyouts

  1. Also updated EuiFlyout to use the new overflow shadow

Which also required an extra wrapping div.

Deprecations

  1. The mixins euiOverflowShadowTop and euiOverflowShadowBottom will now be deprecated in favor of euiOverflowShadow.

I also updated the SASS example:

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

cchaos added 4 commits April 12, 2019 11:50
- Overflow shadow now uses the new overlay method (requires an extra wrapper)
- Confirmation modals no longer take up the entire mobile screen
(Required extra wrapping div)
@cchaos cchaos added the deprecations Contains deprecations. Add them to the deprecations meta ticket after merge. label Apr 12, 2019
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Ran locally, but did not check IE. Might want to check there because of some of the width calcs. We need to be careful about when this goes into Kibana because I think it will cause breaks because of the extra wrapping divs. I don't think we need to label it as breaking, but let's just make sure we do the Kiana PR immediately when this merges so someone in engineering doesn't casually add this in.

@@ -1,7 +1,12 @@
.euiFlyoutBody {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder. The changes you've made here will require you to likely follow up on the Kibana PR for search profiler. We might want to do some searches against CSS selectors for modal and flyout in Kibana. Without the inner div you're using my guess is those HTML only versions will break.

src/components/modal/_modal.scss Outdated Show resolved Hide resolved
src/global_styling/mixins/_shadow.scss Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

👍 Really nice solution, love the anchored-to-bottom look, reminds me of an iOS action sheet.

Since you are in the neighborhood, might be good to add a Snippet to these docs.

@cchaos
Copy link
Contributor Author

cchaos commented Apr 12, 2019

but did not check IE. Might want to check there

Already did, everything is as it was

We need to be careful about when this goes into Kibana

My plan before this merges is to link it against Kibana and make sure all the instances work

@cchaos
Copy link
Contributor Author

cchaos commented Apr 15, 2019

So I linked into Kibana, and the good thing is that my hack in Search Profiler is the only use of the flyout or modal classes directly (in Kibana) so that's just a quick fix when we update Kibana's EUI version. This could affect other consumers, though, but I don't think it's a breaking change since that's now how we support EUI usage.

I will want to do a more overall Modal and Flyout usage consolidation since there are some pages that were converted early on that hasn't evolved. Also there are some pages that really need to have their mobile sizes looked at.

Example:

Screen Shot 2019-04-15 at 11 23 30 AM

@cchaos cchaos merged commit d6c1bcc into elastic:master Apr 15, 2019
@cchaos cchaos deleted the better-overflow-shadow branch April 15, 2019 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Contains deprecations. Add them to the deprecations meta ticket after merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants