-
Notifications
You must be signed in to change notification settings - Fork 841
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
[Emotion] Convert EuiPanel #5891
Conversation
- Removed explicit `paddingSize` and `borderRadius` classnames. - Fixed `useEuiPaddingCSS` and `useEuiBackgroundColorCSS` to render `css` blocks so consuming components render the key name in the class - Fixed default size value of `euiShadow` - Moved `panelPaddingValues` to be declared in resizable_panel
…reviously supported sizes - Fixes popovers and cards
- Fixe EuiCallOut description margin when only child
Preview documentation changes for this PR: https://eui.elastic.co/pr_5891/ |
src/components/panel/panel.style.ts
Outdated
radius: { | ||
none: css``, | ||
m: css` | ||
border-radius: ${euiTheme.border.radius.medium}; | ||
`, | ||
}, |
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.
New pattern of nested styling based on prop values. This allowed me to write
styles.radius[borderRadius]
Since the key name would conflict with sizing or any other t-shirt type prop values without having to re-create a specific string. The only downside is that it sill only renders the final key like -m
in the class name. So you could be situations where its -m-l
not knowing which is for radius vs size.
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.
Super cool pattern! takes notes
The downside doesn't overly bother me, people probably shouldn't be looking at our classNames for semantic meaning in any case 🤔
src/components/panel/panel.tsx
Outdated
import { euiPanelStyles } from './panel.style'; | ||
|
||
export const SIZES = PADDING_SIZES; | ||
export type PanelPaddingSize = EuiPaddingSize; |
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.
EuiPanel now gets its possible padding size values and styles from a single utility. 🙌
# Conflicts: # src/components/datagrid/controls/__snapshots__/column_selector.test.tsx.snap # src/components/datagrid/controls/__snapshots__/column_sorting.test.tsx.snap # src/components/form/super_select/__snapshots__/super_select.test.tsx.snap # src/components/popover/__snapshots__/popover.test.tsx.snap # src/components/tour/__snapshots__/tour_step.test.tsx.snap
Preview documentation changes for this PR: https://eui.elastic.co/pr_5891/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5891/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5891/ |
Lol I assumed Greg had this - I'll grab this review tomorrow! |
…roundColorCSS snapshots
Preview documentation changes for this PR: https://eui.elastic.co/pr_5891/ |
src/components/datagrid/controls/__snapshots__/column_selector.test.tsx.snap
Outdated
Show resolved
Hide resolved
@@ -449,7 +449,7 @@ exports[`props options list is rendered 1`] = ` | |||
</div> | |||
</div> | |||
<div | |||
class="euiPanel euiPanel--borderRadiusMedium euiPanel--plain euiPanel--noShadow euiComboBoxOptionsList euiPopover__panel euiPopover__panel-isAttached euiPopover__panel-noArrow euiPopover__panel-isOpen euiPopover__panel--bottom" | |||
class="euiPanel euiPanel--plain euiComboBoxOptionsList euiPopover__panel euiPopover__panel-isAttached euiPopover__panel-noArrow euiPopover__panel-isOpen euiPopover__panel--bottom css-1pupmsc-euiPanel-grow-m-plain" |
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.
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.
More snapshot updates coming with the removal of the padding class 🤣
That's all my comments I think! The rest LGTM! |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5891/ |
# Conflicts: # src/components/datagrid/controls/__snapshots__/column_sorting.test.tsx.snap
- Renamed `PanelPaddingSize` to `PanelPaddingSize_Deprecated` and added comment - Removed padding class names
Preview documentation changes for this PR: https://eui.elastic.co/pr_5891/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5891/ |
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.
🎉
Thank you @constancecchen ! |
Lots of snapshot updates
EuiPanel
borderRadius
classnamespanelPaddingValues
to be declared in resizable_paneluseEuiPaddingCSS
utility, EuiPanel's now have two more options for paddingxs
andxl
hasBorder
andhasShadow
euiPanel()
mixin to be theglobal_styles/
versionOther fixes
overflow: hidden
Utility fixes
useEuiPaddingCSS
anduseEuiBackgroundColorCSS
to rendercss
blocks so consuming components render the key name in the classeuiShadow
Checklist
[ ] Added documentation[ ] Checked Code Sandbox works for any docs examples[ ] Checked for breaking changes and labeled appropriately[ ] Checked for accessibility including keyboard-only and screenreader modes