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

[Amsterdam] Improve modals #3515

Merged
merged 7 commits into from
May 29, 2020

Conversation

johnbarrierwilson
Copy link
Member

@johnbarrierwilson johnbarrierwilson commented May 26, 2020

Summary

This PR removes the 1px border on modal panels to better match the rest of the k8/amsterdam theme. It also improves overlay color by going with a darker shade instead of lighter.

Before After
image image

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
    - [ ] Props have proper autodocs
    - [ ] Added documentation examples
    - [ ] Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@johnbarrierwilson johnbarrierwilson marked this pull request as ready for review May 26, 2020 15:35
@kibanamachine
Copy link

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

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.

I really like the way this draws attention away from the normal content and to the overlaid component (modal, flyout, etc). However, I'm wondering if this should be an option within the component as to whether to render dark or light.

For example, there's one component usage of EuiOverlayMask that the darker background doesn't work with; EuiImage. When in full screen mode, the component displays the caption below the image in regular text color (dark gray). It's no longer visible with this darker mask.

Screen Shot 2020-05-27 at 14 37 02 PM

@snide
Copy link
Contributor

snide commented May 27, 2020

However, I'm wondering if this should be an option within the component as to whether to render dark or light.

I'd stick with one or the other if we can. Otherwise we'll see too much subjectivity in the decisions downstream. Either is fine. The answer might just be to adjust the caption to have a background to call it out so that it works there.

I'm indifferent to the actual coloring itself. It's very subjective which style people prefer.

@cchaos
Copy link
Contributor

cchaos commented May 27, 2020

Yeah I waffled a bit about suggesting that. I can see arguments for both. I would hate though to have to force a background color on the caption (box within box effect). Maybe just another Amsterdam override.

@johnbarrierwilson
Copy link
Member Author

How's this? Using $euiColorGhost as the caption color regardless of theme.

Light Theme Dark Theme WCAG
image image image

@kibanamachine
Copy link

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

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.

😎 Cool! Thanks for making that change. LGTM!

@johnbarrierwilson johnbarrierwilson merged commit 29174cb into elastic:master May 29, 2020
@johnbarrierwilson johnbarrierwilson deleted the amsterdam/modals branch May 29, 2020 18:11
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.

4 participants