Fullscreen modal margin#2567
Conversation
|
Visual regression report✅ No changes.
Baselines come from the |
matyasf
left a comment
matyasf
left a comment
There was a problem hiding this comment.
Please write a "How to test" part in the PR description too
| * Inline styles applied to the root element. Merged with the default | ||
| * `borderRadius: inherit` so consumers can override it. | ||
| */ | ||
| style?: React.CSSProperties |
There was a problem hiding this comment.
We do not allow to set users CSS for our components. It opens up way too many possibilities leading to inconsistent design and bugs for users during internal styles changes.
We could pass have a borderRadius prop if needed (that defaults to inherit)
| componentDidMount() { | ||
| this.props.makeStyles?.() | ||
| window.addEventListener('resize', this.updateHeight) | ||
| window.addEventListener('resize', this.updateSize) |
There was a problem hiding this comment.
We might need some conditionals here, window does not exists in SSR environments an in Jest unit tests
| } = this.props | ||
|
|
||
| const isFullScreen = size === 'fullscreen' | ||
| const isSmallScreen = this.state.windowWidth < 480 // 30em at 16px base |
There was a problem hiding this comment.
A bit off, but in the previous styling enginer breakpoints were defined at theme level. This might be better for us too, so we dont have components changing visuals lots of times when resizing a window.
3 participants
Modal fixes:
fullScreenMargintoken adds separation from the viewport edge to fix that a11y issue on bigger screen sizes.borderRadiusfrom the component theme wasn't applying because Dialog's hardcoded inlineborderRadius: inheritalways overrode the Emotion CSS class. Dialog now accepts a style prop so consumers can override that default.