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.

Sign up for GitHub

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
Merged

Adds maxWidth prop to EuiModal #1165

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ff044b
add maxWidth prop to EuiModal
ryankeairns Sep 5, 2018
c11b1cd
switch default to true
ryankeairns Sep 6, 2018
60a218a
Merge branch 'master' into rk/euimodal-max-width
Sep 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src-docs/src/views/modal/modal_example.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export const ModalExample = {
props: { EuiConfirmModal },
demo: <ConfirmModal />,
}, {
title: 'Overflow overflow test',
title: 'Overflow test',
source: [{
type: GuideSectionTypes.JS,
code: overflowTestSource,
Expand All @@ -72,7 +72,7 @@ export const ModalExample = {
}],
text: (
<p>
This demo is to test long overflowing body content.
This demo is to test long overflowing body content. It also demonstrates the <EuiCode>maxWidth</EuiCode> property.
</p>
),
props: { EuiConfirmModal },
Expand Down
2 changes: 1 addition & 1 deletion src-docs/src/views/modal/overflow_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export class OverflowTest extends Component {
<EuiOverlayMask>
<EuiModal
onClose={this.closeModal}
style={{ width: '800px' }}
maxWidth
>
<EuiModalHeader>
<EuiModalHeaderTitle >
Expand Down
4 changes: 4 additions & 0 deletions src/components/modal/_modal.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@
}
}

.euiModal--maxWidth-default {
max-width: map-get($euiBreakpoints, "m");
}

.euiModal--confirmation {
width: 450px;
min-width: auto;
Expand Down
30 changes: 29 additions & 1 deletion src/components/modal/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,21 @@ export class EuiModal extends Component {
className,
children,
onClose, // eslint-disable-line no-unused-vars
maxWidth,
style,
...rest
} = this.props;

const classes = classnames('euiModal', className);
let newStyle;
let widthClassName;
if (maxWidth === true) {
widthClassName = 'euiModal--maxWidth-default';
} else if (maxWidth !== false) {
const value = typeof maxWidth === 'number' ? `${maxWidth}px` : maxWidth;
newStyle = { ...style, maxWidth: value };
}

const classes = classnames('euiModal', widthClassName, className);

return (
<FocusTrap
Expand All @@ -43,6 +54,7 @@ export class EuiModal extends Component {
className={classes}
onKeyDown={this.onKeyDown}
tabIndex={0}
style={newStyle || style}
{...rest}
>
<EuiButtonIcon
Expand All @@ -65,4 +77,20 @@ EuiModal.propTypes = {
className: PropTypes.string,
children: PropTypes.node,
onClose: PropTypes.func.isRequired,
/**
* Sets the max-width of the modal.
* Set to `false` to not restrict the width,
* set to `true` to use the default (`euiBreakpoints 'm'`),
* set to a number for a custom width in px,
* set to a string for a custom width in custom measurement.
*/
maxWidth: PropTypes.oneOfType([
PropTypes.bool,
PropTypes.number,
PropTypes.string,
]),
};

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 •
edited
Loading

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.

};