[Emotion] Convert EuiFlyout
#6108
Conversation
Merge Changes from EUI Main
Merging in the latest code from eui/main
Merging in the latest code from main
Merging in the latest code from the main branch
Pulling in the latest code from the main branch
Merging in the latest code from main
…key in the euiTheme
…dated euiOverflowShadowStyles to be an export from _helpers.ts to be used as a styling funciton within EuiFlyout
…utHeader, EuiFlyoutFooter, and EuiFlyoutBody
…lyout Merging in the latest code from the main branch
…lyout Pulling in code from main branch
…lyout Merging in the latest code from main
…ts. Code clea nup
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6108/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6108/ |
…dating the flmenu close by clicking on the overlay mask was not triggering. Updated this test to click a specific position on the screen to trigger the click
…pear inside of the flyout instead of outside. Updated Collapsible Nav snapshots
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6108/ |
| @@ -54,6 +55,7 @@ export type EuiThemeShape = { | |||
| animation: _EuiThemeAnimation; | |||
| breakpoint: _EuiThemeBreakpoints; | |||
| levels: _EuiThemeLevels; | |||
| form: _EuiThemeForm; | |||
There was a problem hiding this comment.
I'm not totally clear on why this needs its own theme level variable rather than us having an exported maxWidth variable within src/components/form - @thompsongl any thoughts on that?
There was a problem hiding this comment.
I agree; no need to extend the global theme object.
There was a problem hiding this comment.
@breehall Re-ping on this - can you remove the new form variable from EuiTheme and instead just export euiFormMaxWidth from src/components/form/form.styles.ts?
…iFlyoutHeader styles in flyout.styles.ts
…lyouBody styles in flyout.styles.ts
…iFlyouFooter styles in flyout.styles.ts
… as their styles have been moved to flyout.styles.ts
…nd resolved merge conflicts. Also updated snapshots for collapsible_nav
… for Flyout child components
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6108/ |
…the Flyout context. There's no need for these items to be split anymore. Updated references to flyout types in flyout test files
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6108/ |
| import { EuiButtonIcon, EuiButtonIconPropsForButton } from '../button'; | ||
| import { EuiFocusTrap, EuiFocusTrapProps } from '../focus_trap'; | ||
| import { EuiOverlayMask, EuiOverlayMaskProps } from '../overlay_mask'; | ||
|
|
||
| import { CommonProps, keysOf, PropsOfElement } from '../common'; |
There was a problem hiding this comment.
[Not a change request, just a comment/suggestion for the future] The number of lines in this file that appear to have been arbitrarily moved around are somewhat confusing/create a decent amount of diff noise. I assume they occurred when you moved types back out of a separate file into this file - in the future, I'd strongly suggest making use of git revert to accomplish undoing code changes as it can help reduce diffs. This in turn helps reduce the number of things code reviewers have to track when reading through code diffs and makes the review experience faster/easier 🤞
| @@ -66,21 +70,21 @@ export type EuiFlyoutSize = typeof SIZES[number]; | |||
| * Custom type checker for named flyout sizes since the prop | |||
| * `size` can also be CSSProperties['width'] (string | number) | |||
| */ | |||
| function isEuiFlyoutSizeNamed(value: any): value is EuiFlyoutSize { | |||
| export function isEuiFlyoutSizeNamed(value: any): value is EuiFlyoutSize { | |||
There was a problem hiding this comment.
[change request] I assume these exports came from the type file undo as well. Can we please remove them if we're not actually importing the fns/vars anywhere?
| export const sideToClassNameMap = { | ||
| left: 'euiFlyout--left', | ||
| right: null, | ||
| }; |
There was a problem hiding this comment.
No instances of euiFlyout-- references/overrides in Kibana anywhere, so IMO we're safe to remove these classNames and maps entirely.
| export const sideToClassNameMap = { | |
| left: 'euiFlyout--left', | |
| right: null, | |
| }; |
| export const paddingSizeToClassNameMap = { | ||
| none: 'euiFlyout--paddingNone', | ||
| s: 'euiFlyout--paddingSmall', | ||
| m: 'euiFlyout--paddingMedium', | ||
| l: 'euiFlyout--paddingLarge', | ||
| }; | ||
|
|
||
| export const PADDING_SIZES = keysOf(paddingSizeToClassNameMap); | ||
| type _EuiFlyoutPaddingSize = typeof PADDING_SIZES[number]; | ||
| export type EuiFlyoutPaddingSize = typeof PADDING_SIZES[number]; |
There was a problem hiding this comment.
See above
| export const paddingSizeToClassNameMap = { | |
| none: 'euiFlyout--paddingNone', | |
| s: 'euiFlyout--paddingSmall', | |
| m: 'euiFlyout--paddingMedium', | |
| l: 'euiFlyout--paddingLarge', | |
| }; | |
| export const PADDING_SIZES = keysOf(paddingSizeToClassNameMap); | |
| type _EuiFlyoutPaddingSize = typeof PADDING_SIZES[number]; | |
| export type EuiFlyoutPaddingSize = typeof PADDING_SIZES[number]; | |
| export const PADDING_SIZES = ['none', 's', 'm', 'l']; | |
| type _EuiFlyoutPaddingSize = typeof PADDING_SIZES[number]; |
| const classes = classnames( | ||
| 'euiFlyout', | ||
| typeToClassNameMap[type as _EuiFlyoutType], | ||
| sideToClassNameMap[side as _EuiFlyoutSide], | ||
| { | ||
| [`euiFlyout--${type}`]: type, | ||
| [`euiFlyout--${side}`]: side, | ||
| [`euiFlyout--padding-${paddingSize}`]: paddingSize, | ||
| }, | ||
| sizeClassName, | ||
| paddingSizeToClassNameMap[paddingSize as _EuiFlyoutPaddingSize], | ||
| widthClassName, | ||
| className | ||
| ); |
There was a problem hiding this comment.
We shouldn't need to apply any of these modifier classes to EuiFlyout any longer
| const classes = classnames( | |
| 'euiFlyout', | |
| typeToClassNameMap[type as _EuiFlyoutType], | |
| sideToClassNameMap[side as _EuiFlyoutSide], | |
| { | |
| [`euiFlyout--${type}`]: type, | |
| [`euiFlyout--${side}`]: side, | |
| [`euiFlyout--padding-${paddingSize}`]: paddingSize, | |
| }, | |
| sizeClassName, | |
| paddingSizeToClassNameMap[paddingSize as _EuiFlyoutPaddingSize], | |
| widthClassName, | |
| className | |
| ); | |
| const classes = classnames('euiFlyout', className); |
| import React, { | ||
| FunctionComponent, | ||
| HTMLAttributes, | ||
| ReactNode, | ||
| //useContext, | ||
| } from 'react'; |
| // Footer | ||
| euiFlyoutFooter: css` |
There was a problem hiding this comment.
This should be in its own file, flyout_footer.styles.ts and should use BEM namespacing, i.e. euiFlyout__footer
🌟
| // Body | ||
| euiFlyoutBody: css` |
There was a problem hiding this comment.
This should be in its own file, flyout_body.styles.ts and should use BEM namespacing, i.e. euiFlyout__body
| // Header | ||
| euiFlyoutHeader: css` |
There was a problem hiding this comment.
This should be in its own file, flyout_header.styles.ts and should use BEM namespacing, i.e. euiFlyout__header
| export const getFlyoutPadding = ( | ||
| paddingSize: EuiFlyoutPaddingSize, | ||
| { euiTheme }: UseEuiTheme | ||
| ) => { | ||
| const paddingModifiers = { | ||
| none: 0, | ||
| s: euiTheme.size.s, | ||
| m: euiTheme.size.base, | ||
| l: euiTheme.size.l, | ||
| }; | ||
|
|
||
| return paddingModifiers[paddingSize]; | ||
| }; |
There was a problem hiding this comment.
I'm not quite seeing why this needs to be in its own separate file/util - I'd suggest only defining it as a map within flyout.styles.ts and using CSS nesting to target padding of flyout children rather than trying to keep our CSS perfectly flat
|
Due to the nature of the changes, I thought it would be best to bring the actual changes over to a new branch and leave everything else to reduce the diff churn from moving around files. I will be closing this PR and using #6213 moving forward. That PR has a summary and a link to the most recent changes requested. |
Summary
This PR converts
EuiFlyoutto Emotion. There are no functional or UI changes within this conversion.Created
EuiFlyoutcontext as a way to pass the value of thepaddingprop fromEuiFlyoutdown toEuiFlyoutHeader,EuiFlyoutBody, andEuiFlyoutFooter.Checklist
[ ] Props have proper autodocs and playground toggles[ ] Added documentation[ ] Updated the Figma library counterpart