-
Notifications
You must be signed in to change notification settings - Fork 843
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
More EuiPanel options #4504
More EuiPanel options #4504
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_4504/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4504/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4504/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in Chrome, Safari, Firefox, and, Edge and I looked at the code. It LGTM! 🎉
The hasBorder
prop is a great new option for Amsterdam. The shadow was creating a lot of distraction in a few scenarios. One example is the icons doc page. What do you think of changing the panels to have hasBorder={true}
?
I also created a PR cchaos#42 with an idea to solve the hasBorder={true}
example. Let me know what you think.
Panel with border only for amsterdam theme
Preview documentation changes for this PR: https://eui.elastic.co/pr_4504/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️ LGTM! Pulled and [over-] tested locally
Yeah I didn't want to overbloat this PR with more panel usage changes. We can do an audit of our docs usages in a different PR. Maybe in #4449 |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4504/ |
Final 🤞 Updates to EuiPanel
1. Added all the
color
options to the default themeBefore, the only color options the default theme respected were
plain
, andsubdued
. Now it supports all the other colors by also forcing all the non-plain options to remove the border and shadow.2. Added
hasBorder
propThis is mainly to allow the Amsterdam theme, which by default does not include a border, to allow borders. This helps reduce the shadow within shadow issues.
There is some heavy handed allowance of combinations of these props (by ignoring certain props) on purpose because they just don't do well with each other. I've added a callout to the docs to try to get ahead of this and communicate it specifically.
I also changed the name of one of the classes hence the updates to lots of snaps.
Checklist
[ ] Checked for breaking changes and labeled appropriately[ ] Checked for accessibility including keyboard-only and screenreader modes