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

[EuiFlyout] Add paddingSize prop #4448

Merged
merged 9 commits into from
Feb 10, 2021

Conversation

elizabetdev
Copy link
Contributor

@elizabetdev elizabetdev commented Jan 26, 2021

Summary

This PR adds a paddingSize prop to EuiFlyout:

  • When none is passed to the paddingSize the close button it's overlapping the body but I'm assuming consumers are going to pass into the header content with padding. So the overlap is not going to happen.
  • I left the default paddingSize set to l to keep the original paddings. Consumers won't see any change.

sizes@2x

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elizabetdev elizabetdev marked this pull request as ready for review February 2, 2021 16:37
@elizabetdev elizabetdev requested a review from cchaos February 2, 2021 16:39
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Here's a quick component review. Nothing major, looks pretty good overall. I also had an idea for the docs that is easier to just show then to type it out, so here is a PR: elizabetdev#10

Comment on lines 56 to 69
.euiFlyout__closeButton {
@if $paddingAmount == $euiSizeS {
right: $paddingAmount;
top: $paddingAmount;
} @else if $paddingAmount == 0 {
right: $euiSizeS;
top: $euiSizeS;
} @else {
// The actual size of the X button in pixels is a bit fuzzy because of all the
// button padding so there is some pixel pushing here.
right: $paddingAmount - 7px;
top: $paddingAmount - 7px;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would honestly just leave the close button in one place and not move it around with the padding. This way it's always in the same place for the user no matter what the flyout content is. (It also makes this math less complicated 😜 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The close button is now in just one place. 😄

src/components/flyout/_flyout.scss Outdated Show resolved Hide resolved
src/components/flyout/_flyout.scss Outdated Show resolved Hide resolved
src/components/flyout/flyout.tsx Outdated Show resolved Hide resolved
elizabetdev and others added 3 commits February 9, 2021 18:47
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@elizabetdev
Copy link
Contributor Author

Thanks @cchaos, for the suggestions and PR. The example looks much better now, being more interactive.

@kibanamachine
Copy link

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

@elizabetdev elizabetdev requested a review from cchaos February 9, 2021 19:45
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Looks great!! Just one last suggestion on the SASS.

src/components/flyout/_flyout.scss Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@elizabetdev elizabetdev merged commit 5aab25a into elastic:master Feb 10, 2021
@elizabetdev elizabetdev changed the title [Feature Notification] Add EuiFlyout paddingSize prop [EuiFlyout Add paddingSize prop Feb 10, 2021
@elizabetdev elizabetdev changed the title [EuiFlyout Add paddingSize prop [EuiFlyout] Add paddingSize prop Feb 10, 2021
@kibanamachine
Copy link

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

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.

3 participants