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.

Sign up for GitHub

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

Jump to bottom

[Emotion] Convert EuiFlyout #6213

Merged
merged 66 commits into from
Sep 14, 2022
Merged

[Emotion] Convert EuiFlyout #6213

merged 66 commits into from
Sep 14, 2022

Conversation

breehall
Copy link
Contributor

@breehall breehall commented Sep 7, 2022

Summary

This PR converts EuiFlyout to Emotion. There are no functional or UI changes within this conversion.

This PR was started in #6108, but 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.

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • [ ] Props have proper autodocs and playground toggles
  • [ ] Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • [ ] Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@breehall
Removed Sass styling files for EuiFlyout
18d3348
@breehall
Created flyout.styles.ts
61800ce
@breehall
Broke out Flyout Footer styling into its own file and object. Includi…
7efcece 
…ng Flyout Footer padding in the padding matrix in Flyout
@breehall
Moved FlyoutHeader styles out of flyout.styles.tsx and moved them to …
b5f5921 
…their own file. Created a function responsible for composing Flyout padding. This will reduce the amount of code duplications
@breehall
Moved styles for FlyoutBody to a new flyout_body.styles.ts file. Comb…
3e18b05 
…ined padding styles into the new composeFlyoutPadding function
@breehall
Removed Flyout close button styles from the base Flyout styles and mo…
360a9a4 
…ved them into their own function
@breehall
Removed classToMap objects for Flyout sides and paddingSizes
de678da
@breehall
Updated required snapshots and a test case for collapsible_nav
316be14
@breehall
CHANGELOG
a0ac180
@breehall
Copy link
Contributor Author

breehall commented Sep 7, 2022

Summary of the last requested changes
These changes were requested per this review

  1. Remove the FlyoutContext created to pass down the padding to Flyout children
  2. Removed formMaxWidth from EuiTheme and used the version exported from EuiForm
  3. Separated FlyoutHeader, FlyoutBody, and FlyoutFooter styles into their own files with the exception of padding styles. Padding styles are contained within flyout.styles.ts.
  4. Removed many classNameToMap objects as well as modifier classes for Flyout
  5. Pulled the closeButton styles out of the main Flyout styles and created a separate function adjacent to it to compose the styles for the close button
  6. Completely removed flyout_helpers.ts and moved the paddingModiferMap to flyout.styles.ts

I'll be assigning anyone who previously provided a review on #6108 to this PR as well.

@breehall breehall mentioned this pull request Sep 7, 2022
Closed
8 tasks
@breehall breehall marked this pull request as ready for review September 7, 2022 13:48
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

cee-chen
cee-chen reviewed Sep 7, 2022
View reviewed changes
src/components/flyout/flyout.styles.ts Outdated Show resolved Hide resolved
src/components/flyout/flyout.styles.ts Outdated Show resolved Hide resolved
src/components/flyout/flyout.styles.ts Outdated Show resolved Hide resolved
@cee-chen
Copy link
Contributor

cee-chen commented Sep 14, 2022 •
edited
Loading

Quick update: still doing a bit more QA on this. In particular our flyout docs have 0 examples of the side="left" option or closeButtonPosition="outside" so I a) want to add those docs/toggles and b) want to ensure our Emotion conversion of that CSS is working correctly

@breehall
Copy link
Contributor Author

breehall commented Sep 14, 2022 •
edited
Loading

@constancecchen In slack it was decided that we shouldn't have a side: left example in the docs because it's only reserved for navigation. I know it's a toggle and not a full on left side example, but I figured I'd bring it up. I'm cool either way!

@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor

Ahh thanks for that context Bree! in that case I'll likely just do b), which will be local only testing but not commit any docs changes to remote. Thanks!

@cee-chen
Fix close button color
a7e9d45
@cee-chen
Remove unnecessary left/right unsets on outside close buttons
dee610f
@cee-chen
Fix flyout borders
333d272 
- right/left side borders were incorrectly overriding push border thickness and should not be present when type is not `push` present
@cee-chen
Fix euiFlyoutBody__banner CSS
e6b0b1d 
- overflow-x should be on the __banner element, not on EuiCallOut

- remove .euiCallOut CSS completely: it wasn't being applied correctly (spelling), and also the border overrides it was setting no longer apply to Amsterdam themes
@cee-chen
Delete Amsterdam Flyout scss
5536a15
@cee-chen
[EuiFlyoutBody] Misc fixes
289b6ce 
- Remove now-unused modifier class
- Clean up CSS to not use a ternary, which avoids writing 2 sets of CSS styles/overrides
@cee-chen
Merge remote-tracking branch 'upstream/main' into emotion/flyout__redo
485be84
@cee-chen
Use new mathWithUnits helper
5dbe365
@kibanamachine
Copy link

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

@cee-chen
Fix various flyout width issues
937695a 
- fix responsive flyout behavior by removing `max-width: none` from inline styles (was overriding max-width 90vw CSS media query)
- add noMaxWidth Emotion class instead, which can be overridden by said media query

- revert m sized flyouts to `424px` wide min widths
- fix max-width for m sized flyouts missing a `px` unit
- remove unnecessary `Math.round` from l.max

- update inline custom max-width/width styles to use logical properties
@cee-chen
Remove now-unused modifier classes from .euiFlyout__closeButton
04ef1c9
@cee-chen
[misc] add newlines to Emotion styles for readability
52ecb68
@cee-chen
[docs] improve default value documentation
336a505
@cee-chen
Update snapshots
1f93ad2
@cee-chen
[misc] clean up syntax for inline styles
8ad5e9f
@kibanamachine
Copy link

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

cee-chen
cee-chen approved these changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Alrighty, all done QA/ing reviewing! Just wanted to add a couple notes of things I found, and things to look out for in the future:

  1. Flyouts widths were broken/going past the screen width on mobile screens - fixed in 937695a, and totally my fault for recommending that style approach, but also a general reminder to test on mobile

  2. Don't forget to look for & delete Amsterdam overrides (5536a15)

  3. This is probably the main one: preserving CSS comments (a7bbed8). Most CSS comments are important to leave in for context (e.g. the !important in 9253c8a provided context as to why it shouldn't be removed) and as long as the CSS is still relevant/hasn't been removed, we should still be attempting to leave them in.

@kibanamachine
Copy link

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

@breehall breehall merged commit 63027a3 into elastic:main Sep 14, 2022
This was referenced Sep 20, 2022
Closed
Merged
@breehall breehall deleted the emotion/flyout__redo branch September 18, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elizabetdev elizabetdev elizabetdev approved these changes

@cee-chen cee-chen cee-chen approved these changes

@thompsongl thompsongl Awaiting requested review from thompsongl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@breehall @kibanamachine @cee-chen @thompsongl @elizabetdev