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

Jump to bottom

Give close button on flyout a data-test-subj #1000

Merged
merged 5 commits into from
Jul 11, 2018
Merged

Give close button on flyout a data-test-subj #1000

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
48b0af1
Give close button on flyout a data-test-subj
stacey-gammon Jul 11, 2018
389a934
update changelog
stacey-gammon Jul 11, 2018
df10dc5
Merge branch 'master' into 2018-07-11-data-test-subj-for-flyout-close
stacey-gammon Jul 11, 2018
0f63bb5
Update jest snapshots and improve changelod message
stacey-gammon Jul 11, 2018
da100cd
add space
stacey-gammon Jul 11, 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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
## [`master`](https://github.com/elastic/eui/tree/master)

No public interface changes since `1.2.1`.
- Gave Flyout close button a data-test-subj ([#1000](https://github.com/elastic/eui/pull/1000/files))
Copy link
Contributor

@cjcenizal cjcenizal Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe change this to:

- Gave `EuiFlyout` close button a `data-test-subj` etc...

for consistency?


## [`1.2.1`](https://github.com/elastic/eui/tree/v1.2.1)

Expand Down
29 changes: 8 additions & 21 deletions src/components/flyout/flyout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import React, {
Component,
} from 'react';
import React, { Component } from 'react';
import classnames from 'classnames';
import PropTypes from 'prop-types';
import FocusTrap from 'focus-trap-react';
Expand Down Expand Up @@ -28,21 +26,9 @@ export class EuiFlyout extends Component {
};

render() {
const {
className,
children,
hideCloseButton,
onClose,
ownFocus,
size,
...rest
} = this.props;
const { className, children, hideCloseButton, onClose, ownFocus, size, ...rest } = this.props;

const classes = classnames(
'euiFlyout',
sizeToClassNameMap[size],
className
);
const classes = classnames('euiFlyout', sizeToClassNameMap[size], className);

let closeButton;
if (onClose && !hideCloseButton) {
Expand All @@ -53,14 +39,17 @@ export class EuiFlyout extends Component {
color="text"
aria-label="Closes this dialog"
onClick={onClose}
data-test-subj="euiFlyoutCloseButton"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something that can be passed down via {...props}? Like if we added closeButtonProps that accepted an object of props and here just spread them into the close button.

My worry with hard-coding this value is if there are multiple flyouts on the page, it might not know which close button is being targeted and so you could get a false pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea that's a downside. We could do that too. I don't feel strongly either way, though if we do go that route, we could put onClose in that too (but then it should be onClick) closeButtonProps = { onClick, data-test-subj, aria-label }.

I'm game with either.

Copy link
Contributor

@cjcenizal cjcenizal Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is the kind of situation where it makes sense to hard-code the data-test-subj. We do the same thing for the close button in toasts and for the clear and toggle buttons in the combo box, and rely on the consumer to add a scoping data-test-subj to the particular instance of the component itself to avoid conflicts.

/>
);
}

const flyoutContent = (
<div
role="dialog"
ref={node => { this.flyout = node; }}
ref={node => {
this.flyout = node;
}}
className={classes}
tabIndex={0}
onKeyDown={this.onKeyDown}
Expand All @@ -75,9 +64,7 @@ export class EuiFlyout extends Component {
// to click it to close it.
let optionalOverlay;
if (ownFocus) {
optionalOverlay = (
<EuiOverlayMask onClick={onClose} />
);
optionalOverlay = <EuiOverlayMask onClick={onClose} />;
}

return (
Expand Down