[EuiNotificationEvent] Render icon instead of button if onRead is undefined #4881
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4881/ |
There was a problem hiding this comment.
Thanks @wenchonglee,
The PR is looking good. 🎉
Can you improve the following description that you can find here
src-docs/src/views/notification_event/notification_event_example.js?
And then there's a small change that I'm requesting in the code.
| <EuiIcon | ||
| type="dot" | ||
| aria-label={iconAriaLabel} | ||
| title={iconTitle} |
There was a problem hiding this comment.
🤔 We could only use the title prop here. It would create an aria-labelledby attribute and wire it to the title tag inside the SVG. But then I think just read/unread as a title wouldn't be too descriptive for screen readers. And {eventName} is read would be good for screen readers but too long to display on hover.
So I think the way you did it by adding aria-label and title is better. What do you think @myasonik?
There was a problem hiding this comment.
@miukimiu Your thinking on it perfect – exactly! 👍
|
jenkins test this |
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4881/ |
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4881/ |
There was a problem hiding this comment.
I noticed a few more things. After you fix those I'm happy to approve the PR. We should also wait for @myasonik to check the a11y and then we good to merge. 😄
| eventName, | ||
| }) => { | ||
| const classesReadState = classNames('euiNotificationEventReadIcon', { | ||
| 'euiNotificationEventReadButton--isRead': isRead, |
There was a problem hiding this comment.
In EUI we don't use a class that belongs to a different component. So this class should be specific to this component:
| 'euiNotificationEventReadButton--isRead': isRead, | |
| 'euiNotificationEventReadIcon--isRead': isRead, |
Then you need to add the styles .euiNotificationEventReadIcon--isRead svg into src/components/notification/_notification_event_read_icon.scss
There was a problem hiding this comment.
Thanks for the review! I wasn't sure about the practice for isolating classes.
I definitely missed the types though 😅
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4881/ |
|
jenkins test this |
|
jenkins test this |
There was a problem hiding this comment.
Thanks @wenchonglee,
I fixed the icon to be vertical centered.
I also tested locally in Chrome, Safari, Edge, and Firefox, and LGTM! 🎉
Let's just wait for @myasonik approval and we'll be ready to merge.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4881/ |
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4881/ |
Changes for #4791
Summary
How it looks like, with the bottom notification not having
onReadI initially had an if-else statement in
<EuiNotificationEventReadButton>to render an icon or button. But we'd have double theuseEuiI18nhooks and I decided to create another internal component<EuiNotificationEventReadIcon>A couple of concerns I have because I'm very unfamiliar with accessibility and i18n:
{eventName} is readas a guessDo let me know what you think, or feel free to close the PR if it is something that should be resolved by the Eui team instead.
Checklist
Props have proper autodocs and playground togglesAdded documentationChecked Code Sandbox works for the any docs examples