-
Notifications
You must be signed in to change notification settings - Fork 843
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
Give close button on flyout a data-test-subj #1000
Give close button on flyout a data-test-subj #1000
Conversation
@@ -53,14 +39,17 @@ export class EuiFlyout extends Component { | |||
color="text" | |||
aria-label="Closes this dialog" | |||
onClick={onClose} | |||
data-test-subj="euiFlyoutCloseButton" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Was the reformatting done automatically by your IDE @stacey-gammon?
Looks like the Jest snapshots need to be updated. |
CHANGELOG.md
Outdated
@@ -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)) |
There was a problem hiding this comment.
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?
yep. I have since turned it off for the eui repo because it reformatted all of CHANGELOG.md. |
Re: jest snapshots. Can you walk me through that @cjcenizal ? Looks like the commands are a bit different for eui vs Kibana. I ran
don't see anything in contributing doc re: how to run jest tests. |
nm i found the docs. I need to read more thoroughly. :) |
Oh good because I was just about to look for them! 😄 |
Will be useful for elastic/kibana#20163