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

Fix bug with EuiFlyout maxWidth interfering with responsive mode. #1124

Merged
merged 4 commits into from
Aug 17, 2018

Conversation

cjcenizal
Copy link
Contributor

Fixes #1122

I also added examples for the maxWidth prop and the large size flyout. @elastic/kibana-design In general, are we still trying to add examples to the docs site whenever we add new props?

@@ -62,9 +62,9 @@ export class Flyout extends Component {
>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="m">
<h1 id="flyoutTitle">
<h2 id="flyoutTitle">
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 updated these examples to use h2 because in most cases I've seen this heading won't make sense as a top-level heading; instead it will make more sense as a child of the top-level.

@cjcenizal cjcenizal requested review from cchaos and snide August 17, 2018 15:23
@snide
Copy link
Contributor

snide commented Aug 17, 2018

are we still trying to add examples to the docs site whenever we add new props

Yes. Sometimes the smaller ones we document (as we did in the header) here. But you're right, it helps catch some of these issues. Thanks for adding it.

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.

Good spot CJ. Tested this in browser and looked over the code. Thanks for adding the example.

],
text: (
<p>
In this example, we set <EuiCode>maxWidth</EuiCode> to <EuiCode>33vw</EuiCode>, to
Copy link
Contributor

@snide snide Aug 17, 2018

Choose a reason for hiding this comment

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

Very minor thing, but I'd use an example with a pixel value (a good example one to show for the forms is 448px, which is the max-width of the form inputs + 24px padding on each side). The reason you often need the maxWidth prop is that the more fluid default widths (which are vw) can be inconsistent. Usually you're over riding it to make sure that thinner content doesn't get too airy on a wide screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll update the example with that pixel value. FYI, most of the time I've used a flyout in Kibana the small has been too cramped (on smaller screens like laptop) and the large has been too obtrusive (on all screens, both laptop and wide monitor). The pixel value you suggested works great.

@cjcenizal
Copy link
Contributor Author

@snide That px width uncovered another bug. The min-width value can fight with the max-width value:

image

I'm not sure how we want to solve this one, but we can figure it out in another PR. 😄

@snide
Copy link
Contributor

snide commented Aug 17, 2018

That's fine. TBH, it's not really a bug. It's using the settings provided.

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.

This fix looks good to me. I just had a couple comments, one about the docs example usage, and one about the sass comments.

<EuiFlyout
onClose={this.closeFlyout}
aria-labelledby="flyoutMaxWidthTitle"
maxWidth="448px"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change the examples to not pass the 'px'? I intentionally wanted to mimic the way React handles the styles prop where if you send a number maxWidth={448}, it would treat it as a px value. But if you send a string maxWidth="40rem" it would apply it as is. I was thinking this would be an easier way for consumers to pass values without having to remember to append 'px'.

@@ -67,16 +63,23 @@ $flyoutSizes: (
}
}

/**
* 1. Calculating the minimum width based on the screen takover breakpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

The 1. comment here references code above it, can you separate them or move it back to before the map?

@cjcenizal
Copy link
Contributor Author

That's fine. TBH, it's not really a bug. It's using the settings provided.

Heh but you can't make the flyout hug the form!

@cjcenizal cjcenizal merged commit 5a0ff16 into elastic:master Aug 17, 2018
@cjcenizal cjcenizal deleted the flyout-max-width-fix branch August 17, 2018 19:56
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