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

Added maxWidth prop to EuiFlyout #1090

Merged
merged 5 commits into from
Aug 7, 2018
Merged

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Aug 6, 2018

Fixes #1071

Similarly to how EuiPage uses the restrictWidth prop, the maxWidth prop can be:

  • false (default): No max-width
  • true: Adds a pre-defined max width depending on flyout size
  • number: Will add the number to the style prop as a maxWidth value in px
  • string: Will add the full string as the maxWidth value in whatever measurement it provides

The string option has now also been added to EuiPage and EuiPageBody as well as the simplified style js commented below, including removing the unnecessary --widthIsNotRestricted classes, and adding this prop to the TS definitions.

@cchaos cchaos requested review from snide and chandlerprall August 6, 2018 20:58
} else if (maxWidth !== false) {
// if style has been passed as a prop, add to it
if (style) {
newStyle = style;
Copy link
Contributor

Choose a reason for hiding this comment

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

Best to create a new style object here,

newStyle = { ...style, maxWidth };

otherwise this will add the maxWidth key/value to the original style prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also get confused about JS's stupid array reference stuff and forget about the spread method. Just so I understand, does the way your method work is that it will add the key/value pair of maxWidth or is it just adding that value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow, that actually completely negates the need for the if statement, because if style is empty, then there's just no other key/value pairs. Nice!

-  Added `string` to allowed `restrictWidth` prop type of `EuiPage` and `EuiPageBody`
- Fixed wrong class name being added to `EuiPageBody` when `restrictWidth !== false`
- Added these props to their TS defs
@cchaos
Copy link
Contributor Author

cchaos commented Aug 6, 2018

@chandlerprall Can you take another look. I also updated EuiPage and EuiPageBody to utilize the same type of logic.

Can you also check that I added the TS stuff correctly?

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM!; reviewed code changes

@chandlerprall
Copy link
Contributor

@cchaos all looks good to me (including TS defs), thanks for the changes!

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Tested in browser by making my own example. Verified docs and review code.

I like that we're standardizing this kind of variety from a props perspective. It's a nice pattern.

@cchaos cchaos merged commit 4cfa4c9 into elastic:master Aug 7, 2018
@cchaos cchaos deleted the fly-out-max-width branch August 7, 2018 19:25
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