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

Add size properties to EuiModal #1164

Closed

Conversation

ryankeairns
Copy link
Contributor

Fixes #1154

Adds height, maxHeight, width, and maxWidth to EuiModal.

The need for these properties occurrs in Canvas where users can search/filter table (workpad list) and grid (element types) content thus creating a situation where the height of theEuiModalBody content is dynamic.

To prevent the modal from being 'jumpy' (see https://github.com/elastic/kibana-canvas/issues/869 ) , we apply a height (in vh) to the modal. Related, since we're using vh, we don't want the modal to get too tall on large displays so we limit modal height using max-height.

🐉
fixed-modal-height

@ryankeairns ryankeairns self-assigned this Sep 5, 2018
width: false,
maxWidth: false,
height: false,
maxHeight: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if they set any of these to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has no impact. The boolean default was used here as a means for accepting and evaluating multiple PropTypes, allowing users to input a string or a number. The latter of which would have the recommended measurement applied to it.

There is similar logic in the flyout component that I was referring to.

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.

Sorry for my confusion, but why are we asking them to explicitly add these props when we just apply them to the style prop? Can't they just add them directly to style? It doesn't seem like this does anything more than the default style prop does?

@cchaos
Copy link
Contributor

cchaos commented Sep 5, 2018

In the past we've added things like this because we were providing defaults with the option to set custom values. So when they passed a value, we would add a class and set a default value in CSS. This just passes it directly back to the style prop.

@ryankeairns
Copy link
Contributor Author

The original discussion is here --> https://github.com/elastic/kibana-canvas/pull/1078#pullrequestreview-151207920

@cchaos I had similar feelings as I was building this out - realizing there are no defaults and that it just passes to style - but thought it was worth the convenience (and discussion) presuming this could be a fairly common scenario.

That said, maybe the default here is to set the height to 75vh and the prop is something like fixedHeight: true(Note: 75vh is the height set on euiModal_flex). Then we could leave the max-height style override up to the app/team implementing the modal.

@ryankeairns
Copy link
Contributor Author

On second thought, I'm not sure a blanket 75vh is a good default. It seems the desired height would vary based upon each specific situation.

Perhaps a fixed height modal isn't a common enough use case to add these props, I suppose that is the core question here.

@ryankeairns ryankeairns closed this Sep 5, 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.

EuiModal add height or min-height prop
2 participants