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.
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
[EuiNotificationEvent] Render icon instead of button if onRead is undefined #4881
[EuiNotificationEvent] Render icon instead of button if onRead is undefined #4881
Changes from 8 commits
23498f1
354d2ff
e035ea0
3c39956
dd6ca6f
98a2ea5
6c8f511
e44709f
b692802
4ac7368
69543b9
c592603
b1f03a2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
In EUI we don't use a class that belongs to a different component. So this class should be specific to this component:
Then you need to add the styles
.euiNotificationEventReadIcon--isRead svg
intosrc/components/notification/_notification_event_read_icon.scss
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.
Thanks for the review! I wasn't sure about the practice for isolating classes.
I definitely missed the types though 😅
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.
🤔 We could only use the
title
prop here. It would create anaria-labelledby
attribute and wire it to thetitle
tag inside the SVG. But then I think justread/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
andtitle
is better. What do you think @myasonik?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.
@miukimiu Your thinking on it perfect – exactly! 👍