[Amsterdam] Improve modals#3515
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3515/ |
There was a problem hiding this comment.
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.
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. |
|
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. |
|
How's this? Using
|
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3515/ |
cchaos
left a comment
There was a problem hiding this comment.
😎 Cool! Thanks for making that change. LGTM!
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.
Checklist
- [ ] Props have proper autodocs- [ ] Added documentation examples- [ ] Added or updated jest tests- [ ] Checked for accessibility including keyboard-only and screenreader modes