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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
- Added `zIndexAdjustment` to `EuiPopover` which allows tweaking the popover content's `z-index` ([#1097](https://github.com/elastic/eui/pull/1097))
- Added new `EuiSuperSelect` component and `hasArrow` prop to `EuiPopover` ([#921](https://github.com/elastic/eui/pull/921))

**Bug fixes**

- `EuiFlyout` responsive mode now gracefully overrides a custom `maxWidth` ([#1124](https://github.com/elastic/eui/pull/1124)

## [`3.6.1`](https://github.com/elastic/eui/tree/v3.6.1)

- Added TypeScript definition for `findTestSubject` test util ([#1106](https://github.com/elastic/eui/pull/1106))
Expand Down
8 changes: 4 additions & 4 deletions src-docs/src/views/flyout/flyout.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export class Flyout extends Component {
const htmlCode = `<EuiFlyout ...>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="m">
<h1></h1>
<h2></h2>
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
Expand All @@ -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.

A typical flyout
</h1>
</h2>
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
Expand All @@ -85,7 +85,7 @@ export class Flyout extends Component {
return (
<div>
<EuiButton onClick={this.showFlyout}>
Show Flyout
Show flyout
</EuiButton>

{flyout}
Expand Down
6 changes: 3 additions & 3 deletions src-docs/src/views/flyout/flyout_complicated.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,9 @@ export class FlyoutComplicated extends Component {
>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="m">
<h1 id="flyoutComplicatedTitle">
<h2 id="flyoutComplicatedTitle">
Flyout header
</h1>
</h2>
</EuiTitle>
<EuiSpacer size="s" />
<EuiText color="subdued">
Expand Down Expand Up @@ -212,7 +212,7 @@ export class FlyoutComplicated extends Component {
return (
<div>
<EuiButton onClick={this.showFlyout}>
Show Flyout
Show flyout
</EuiButton>

{flyout}
Expand Down
61 changes: 54 additions & 7 deletions src-docs/src/views/flyout/flyout_example.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,17 @@ import { FlyoutComplicated } from './flyout_complicated';
const flyoutComplicatedSource = require('!!raw-loader!./flyout_complicated');
const flyoutComplicatedHtml = renderToHtml(FlyoutComplicated);

import { FlyoutSize } from './flyout_size';
const flyoutSizeSource = require('!!raw-loader!./flyout_size');
const flyoutSizeHtml = renderToHtml(FlyoutSize);
import { FlyoutSmall } from './flyout_small';
const flyoutSmallSource = require('!!raw-loader!./flyout_small');
const flyoutSmallHtml = renderToHtml(FlyoutSmall);

import { FlyoutLarge } from './flyout_large';
const flyoutLargeSource = require('!!raw-loader!./flyout_large');
const flyoutLargeHtml = renderToHtml(FlyoutLarge);

import { FlyoutMaxWidth } from './flyout_max_width';
const flyoutMaxWidthSource = require('!!raw-loader!./flyout_max_width');
const flyoutMaxWidthHtml = renderToHtml(FlyoutMaxWidth);

export const FlyoutExample = {
title: 'Flyout',
Expand Down Expand Up @@ -89,15 +97,15 @@ export const FlyoutExample = {
demo: <FlyoutComplicated />,
},
{
title: 'Flyout sizing and focus',
title: 'Small flyout, ownFocus',
source: [
{
type: GuideSectionTypes.JS,
code: flyoutSizeSource,
code: flyoutSmallSource,
},
{
type: GuideSectionTypes.HTML,
code: flyoutSizeHtml,
code: flyoutSmallHtml,
},
],
text: (
Expand All @@ -107,7 +115,46 @@ export const FlyoutExample = {
also adds background overlay to reinforce your boundries.
</p>
),
demo: <FlyoutSize />,
demo: <FlyoutSmall />,
},
{
title: 'Large flyout',
source: [
{
type: GuideSectionTypes.JS,
code: flyoutLargeSource,
},
{
type: GuideSectionTypes.HTML,
code: flyoutLargeHtml,
},
],
text: (
<p>
In this example, we set <EuiCode>size</EuiCode> to <EuiCode>l</EuiCode>.
</p>
),
demo: <FlyoutLarge />,
},
{
title: 'maxWidth',
source: [
{
type: GuideSectionTypes.JS,
code: flyoutMaxWidthSource,
},
{
type: GuideSectionTypes.HTML,
code: flyoutMaxWidthHtml,
},
],
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.

set the width of the flyout at 33% the width of the viewport.
</p>
),
demo: <FlyoutMaxWidth />,
},
],
};
78 changes: 78 additions & 0 deletions src-docs/src/views/flyout/flyout_large.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import React, {
Component,
} from 'react';

import {
EuiFlyout,
EuiFlyoutHeader,
EuiFlyoutBody,
EuiButton,
EuiText,
EuiTitle,
} from '../../../../src/components';

export class FlyoutLarge extends Component {
constructor(props) {
super(props);

this.state = {
isFlyoutVisible: false,
isSwitchChecked: true,
};

this.closeFlyout = this.closeFlyout.bind(this);
this.showFlyout = this.showFlyout.bind(this);
}

onSwitchChange = () => {
this.setState({
isSwitchChecked: !this.state.isSwitchChecked,
});
}

closeFlyout() {
this.setState({ isFlyoutVisible: false });
}

showFlyout() {
this.setState({ isFlyoutVisible: true });
}

render() {

let flyout;
if (this.state.isFlyoutVisible) {
flyout = (
<EuiFlyout
onClose={this.closeFlyout}
size="l"
aria-labelledby="flyoutLargeTitle"
>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="m">
<h2 id="flyoutLargeTitle">
A large flyout
</h2>
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
<EuiText>
<p>
The large flyout is very wide.
</p>
</EuiText>
</EuiFlyoutBody>
</EuiFlyout>
);
}
return (
<div>
<EuiButton onClick={this.showFlyout}>
Show large flyout
</EuiButton>

{flyout}
</div>
);
}
}
96 changes: 96 additions & 0 deletions src-docs/src/views/flyout/flyout_max_width.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import React, {
Component,
} from 'react';

import {
EuiFlyout,
EuiFlyoutBody,
EuiFlyoutHeader,
EuiButton,
EuiText,
EuiTitle,
EuiCodeBlock,
} from '../../../../src/components';

export class FlyoutMaxWidth extends Component {
constructor(props) {
super(props);

this.state = {
isFlyoutVisible: false,
isSwitchChecked: true,
};

this.closeFlyout = this.closeFlyout.bind(this);
this.showFlyout = this.showFlyout.bind(this);
}

onSwitchChange = () => {
this.setState({
isSwitchChecked: !this.state.isSwitchChecked,
});
}

closeFlyout() {
this.setState({ isFlyoutVisible: false });
}

showFlyout() {
this.setState({ isFlyoutVisible: true });
}

render() {
let flyout;

const htmlCode = `<EuiFlyout ...>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="m">
<h2></h2>
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
...
</EuiFlyoutBody>
</EuiFlyout>
`;

if (this.state.isFlyoutVisible) {
flyout = (
<EuiFlyout
onClose={this.closeFlyout}
aria-labelledby="flyoutMaxWidthTitle"
maxWidth="33vw"
>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="m">
<h2 id="flyoutMaxWidthTitle">
33% wide flyout
</h2>
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
<EuiText>
<p>
For consistency across the many flyouts, please utilize the following code for
implementing the flyout with a header.
</p>
</EuiText>
<EuiCodeBlock language="html">
{htmlCode}
</EuiCodeBlock>
</EuiFlyoutBody>
</EuiFlyout>
);
}

return (
<div>
<EuiButton onClick={this.showFlyout}>
Show max-width flyout
</EuiButton>

{flyout}
</div>
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
EuiTitle,
} from '../../../../src/components';

export class FlyoutSize extends Component {
export class FlyoutSmall extends Component {
constructor(props) {
super(props);

Expand Down Expand Up @@ -47,13 +47,13 @@ export class FlyoutSize extends Component {
ownFocus
onClose={this.closeFlyout}
size="s"
aria-labelledby="flyoutSizeTitle"
aria-labelledby="flyoutSmallTitle"
>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="s">
<h1 id="flyoutSizeTitle">
<h2 id="flyoutSmallTitle">
A small flyout
</h1>
</h2>
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
Expand All @@ -69,7 +69,7 @@ export class FlyoutSize extends Component {
return (
<div>
<EuiButton onClick={this.showFlyout}>
Show Flyout
Show small flyout
</EuiButton>

{flyout}
Expand Down
11 changes: 7 additions & 4 deletions src/components/flyout/_flyout.scss
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@
z-index: 3;
}

/**
* 1. Calculating the minimum width based on the screen takover breakpoint
* 2. Only small flyouts should NOT takover the entire screen
*/
$flyoutSizes: (
"small": (
min: map-get($euiBreakpoints, "m") * .5, /* 1 */
Expand Down Expand Up @@ -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?

* 2. Only small flyouts should NOT takover the entire screen
* 3. If a custom maxWidth is set, we need to override it.
*/
@include euiBreakpoint('xs','s') {
.euiFlyout:not(.euiFlyout--small) { /* 2 */
left: 0;
min-width: 0;
width: auto;
border-left: none;
max-width: 100vw !important; /* 3 */
}

.euiFlyout--small {
width: 80vw; // ensure that it's only partially showing the main content
min-width: 0; /* 2 */
max-width: 80vw !important; /* 3 */
}
}