Add closeButtonLabel to flyout #1031
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cchaos
suggested changes
Jul 19, 2018
src/components/flyout/flyout.js
Outdated
| /** | ||
| * Specify an aria-label for the close button of the flyout. | ||
| */ | ||
| closeButtonLabel: PropTypes.string, |
There was a problem hiding this comment.
Can you rename this to closeButtonAriaLabel so that it's absolutely clear by the prop name itself what the "label" is?
There was a problem hiding this comment.
The other option is to start a closeButtonProps object to handle more than just the aria-label.
There was a problem hiding this comment.
I went with closeButtonAriaLabel, since I think if we allow overwriting everything on the button, consumers might be able to mess around too much with the close button as it is.
src/components/flyout/flyout.js
Outdated
| @@ -98,10 +107,15 @@ EuiFlyout.propTypes = { | |||
| * Locks the mouse / keyboard focus to within the flyout | |||
| */ | |||
| ownFocus: PropTypes.bool, | |||
| /** | |||
| * Specify an aria-label for the close button of the flyout. | |||
cchaos
reviewed
Jul 19, 2018
src/components/flyout/flyout.test.js
Outdated
| expect(label).toBe('Closes this dialog'); | ||
| }); | ||
|
|
||
| test('has a default label for the close button', () => { |
There was a problem hiding this comment.
Should this test read as 'has a custom label for close button'?
There was a problem hiding this comment.
Of course... copy-paste-error :D
cchaos
approved these changes
Jul 19, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Give the
FlyOutcomponent a property to overwrite thearia-labelof the close button, so consumers can make this more specific to their flyout (e.g. relabeling it to: "Close Inspector").