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

Adds maxWidth prop to EuiModal #1165

Merged
merged 3 commits into from
Sep 6, 2018

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Sep 5, 2018

Adds a maxWidth prop to EuiModal that is disabled by default.

When enabled, a default class is applied that sets the max-width to the EUI medium breakpoint value. Users can optionally provide a number value that converts to a max px width value, or they can provide a string that includes an alternative form of measurement (e.g. 90vw).

There is no impact to smaller screen sizes as the medium breakpoint (768px) exceeds the small screen width. For smaller screens, the width is set as 100vw + 2px.

You can test this change using the overflow example in the Modal section of the docs. It was previously set to 800px via style, but it now uses the default width of maxWidth.

@ryankeairns ryankeairns self-assigned this Sep 5, 2018
@ryankeairns ryankeairns requested review from snide and cchaos September 5, 2018 22:00
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.

LGTM. With the one exception.

Long term: I wonder if there's a way to turn this sort of prop functionality into a service?

};

EuiModal.defaultProps = {
maxWidth: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd think we'd want the maxWidth on by default.

Copy link
Contributor Author

@ryankeairns ryankeairns Sep 6, 2018

Choose a reason for hiding this comment

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

Changing it to true would potentially break current implementations, right? Though I suppose it's very likely they would have already set an explicit width else they would have full width modals.

Flipping it to true would be the preferable approach from a design perspective.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it would be breaking or not. I think most implementations, like you said, would have needed to add an explicit width. Since max-width doesn't actually force a width, until a certain size (which is quite large for the default anyway), I would consider it a necessary default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point of reference is heavily Canvas-skewed :) , but it would affect all four instances of modals used in that application. Each is currently set wider than 768px.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, then yeah maybe we do call it a breaking change, but I do think it's an important default moving forward.

@cchaos
Copy link
Contributor

cchaos commented Sep 6, 2018

This isn't really necessary for this PR, but I was just looking at the first example, and it sets a inline style of width: 800px on the modal which makes it really large for it's contents.

screen shot 2018-09-06 at 11 56 51 am

But if you take off the inline width, it seems to only get it's width from the header not the body content.

screen shot 2018-09-06 at 11 58 34 am

Could we, at the very least, change this value to be closer to just containing the form (~400px), or is there a way to better ensure the content of the modal is deciding the width without having to put an explicit value on it?

@ryankeairns
Copy link
Contributor Author

@cchaos Adding words to the switch label does expand the width of the modal (up to the max-width of 400px). Do we want a min-width on modals? or modals containing forms?

screenshot 2018-09-06 11 17 00

@cchaos
Copy link
Contributor

cchaos commented Sep 6, 2018

Oh hmm, ok, then can you just set that particular example to have a width: 400px?

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.

I think it's OK to set the maxWidth setting on by default. Canvas already has something to overwrite it, and AFAIK that's the only place with a super wide modal like that. Using it beyond that, actually goes against our published guidelines so I'm also OK with releasing this as a non-breaking change (with the caveat of us letting cloud know and double checking canvas usage.

@snide
Copy link
Contributor

snide commented Sep 6, 2018

@ryankeairns don't forget a changelog when you do add this.

@ryankeairns
Copy link
Contributor Author

I'll switch it to true. After a quick scan, I see a few other places where this will have an impact, but it looks minimal. In these non-Canvas modals the widths are between 800 and 860 pixels.

@ryankeairns ryankeairns force-pushed the rk/euimodal-max-width branch from a853ce4 to c11b1cd Compare September 6, 2018 18:13
@ryankeairns ryankeairns merged commit d3fb21f into elastic:master Sep 6, 2018
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