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

[EuiPanel] Expanding usages for layout purposes #4194

Merged
merged 11 commits into from
Nov 3, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Oct 28, 2020

I struggled with the title on this one.

Essentially, the aim here was to promote EuiPanel not just as a bordered box, but as a layout container consumers can use to simply provide padding and/or shading to areas of their pages.

This is also the first step towards being able to reuse the component itself in more higher-order components instead of the Sass mixin which creates way more selectors. For this reason, there may look to be some inefficiencies in the SASS but, it's the easiest fix for now.

🔔 The goal

➕ Additions

1. Added color prop

This prop is mainly in preparation for more colored usages by the Amsterdam theme. Currently the default theme will only support plain, subdued, and transparent. While the Amsterdam theme will support all of these:

image

This is always a step towards reusing EuiPanel and these colors for use in EuiCallout.

2. Added borderRadius to turn off rounded corners

The options are simply 'm' or 'none'.

🛠 Updates

3. Changed hasShadow to be true by default but false now fully removes the shadow

Before, EuiPanel always has some sort of a shadow where hasShadow just increased it. More and more we're seeing the need to remove the shadow not increase it. I checked through Kibana and there aren't really any usages of increasing the shadow. So this should be a safe assumption.

Screen Shot 2020-10-28 at 17 46 45 PM

☠️ Set onClick and all beta badge props for deprecation

This was originally applied before we decided to go down the EuiCard path. EuiCard handles the interaction behavior and accessibility of interaction way better (even at all) than EuiPanel. Since EuiCard can now also contain any content, let's let that higher-order component be the one to allow interaction. Similar reasoning behind the beta badge deprecations.

Screen Shot 2020-10-28 at 17 58 30 PM


Most of the rest of the changes are updating downstream usages and fixing up styles to better support the default props. As well as updating docs.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs
  • Added documentation
  • Checked Code Sandbox works for the any docs 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

Comment on lines +230 to +231
'euiCard--shadow', // For matching EuiPanel mixin
'euiCard--borderRadiusMedium', // For matching EuiPanel mixin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a temporary fix/hack until the next step which will be using EuiPanel directly.

Comment on lines +67 to +68
'euiCheckableCard--shadow', // For matching EuiPanel mixin
'euiCheckableCard--borderRadiusMedium', // For matching EuiPanel mixin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

Comment on lines +13 to +15
'transparent': transparent,
'plain': $euiColorEmptyShade,
'subdued': $euiColorLightestShade,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only supporting these colors in the default theme because the others don't quite work well with the border.

@kibanamachine
Copy link

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

@cchaos cchaos mentioned this pull request Oct 28, 2020
35 tasks
Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉 Tested on Chrome, Firefox, Safari, and Edge.

I think it makes sense to have the ability to turn off borderRadius and the shadow. This will allow using this component to create many more types of layouts. I totally support the deprecations.

I just added a few suggestions.

src-docs/src/views/panel/panel_grow.js Outdated Show resolved Hide resolved
src/global_styling/mixins/_panel.scss Outdated Show resolved Hide resolved
src/global_styling/variables/_panel.scss Outdated Show resolved Hide resolved
src-docs/src/views/panel/panel_example.js Outdated Show resolved Hide resolved
cchaos and others added 2 commits October 29, 2020 10:49
Co-authored-by: Elizabet Oliveira <elizabet.oliveira@elastic.co>
@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; viewed & tested docs changes.

@kibanamachine
Copy link

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

@cchaos cchaos merged commit 5d7385c into elastic:master Nov 3, 2020
@cchaos cchaos deleted the ams/panels branch November 3, 2020 18:08
cchaos added a commit to andreadelrio/eui that referenced this pull request Nov 4, 2020
* [EuiPanel] Added `color` and `borderRadius` options
* [EuiPanel] `hasShadow` defaults to `true` but is the slight version
  - color and radius modifiers are now in the mixin
* Fix up usages
* Updated docs and added deprecation notices
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