[Feature Notification] Add EuiNotificationEventMeta by elizabetdev · Pull Request #4433 · elastic/eui

elizabetdev · 2021-01-20T17:15:06Z

Summary

This PR adds the EuiNotificationEventMeta, an inner component that will live inside EuiNotificationEvent.

For testing purposes, I'm adding provisory documentation that is going to be replaced on the next PRs.

Checklist

kibanamachine · 2021-01-20T17:34:17Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

kibanamachine · 2021-01-20T19:26:24Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

kibanamachine · 2021-01-21T15:28:54Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

kibanamachine · 2021-01-21T16:27:24Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

kibanamachine · 2021-01-25T13:57:41Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

kibanamachine · 2021-01-25T20:04:27Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

src/components/notification/notification_event_meta.tsx

cchaos

🎉

Side note: I know that my reviews can be quite long on some of these new components. Let me know if just passing you back a PR is easier than trying to parse through all the comments.

One extra things I would add now instead of later is a playground. While this specific component is still in your head, it's a good candidate for a specific playground setup to check through all the props/combinations.

src-docs/src/views/notification_events/notification_events_example.js

src/components/index.js

src/components/notification/notification_event_read_button.tsx

src/components/notification/notification_event_meta.tsx

src/components/notification/_notification_event_meta.scss

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

kibanamachine · 2021-01-27T13:16:36Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

src/components/notification/notification_event_meta.tsx

elizabetdev · 2021-01-27T18:39:35Z

Thanks, @cchaos. The comments work well because I have the chance to learn by doing by myself. 👍🏽

kibanamachine · 2021-01-27T18:43:58Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

kibanamachine · 2021-01-27T19:16:49Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

elizabetdev · 2021-01-29T17:30:13Z

Thanks, @cchaos! Fixed the badge to grow until 100% of the parent container and truncate. Also fixed the read and unread states to toggle.

cchaos

The only other thing to test for scalability, is if the date/time is very long as well. I think the wrapping of the date to the next line actually isn't too bad, but I'd think you'd want to ensure the overflow menu to stay in the top right.

Also, I'd still highly suggest adding a Playground for the exported components.

--

The next step is maybe to get an a11y by @myasonik . I'm guessing we'll need some more aria tags hooked up to the buttons so they get described by the event. Unless that's coming later.

src/components/notification/_notification_event_meta.scss

myasonik

Thanks for the ping @cchaos – you were spot on!

Just a few minor things that I think you'll need to add a couple more props for but nothing crazy it doesn't seem like.

(Note: I didn't do a full code review. Only reviewed the rendered examples and focused on the areas of code that I needed to target.)

myasonik · 2021-02-02T21:31:28Z

src/components/notification/notification_event_read_button.tsx

+        'euiNotificationEventReadButton.markAsRead',
+        'euiNotificationEventReadButton.markAsUnread',
+      ]}
+      defaults={['Mark as read', 'Mark as unread']}>


This is tricky... We can expect that users will have a lot of this button on the screen so we should try to avoid duplicate button names.

Usually, I'd recommend something like Mark {eventName} as read but we don't have to available in this component.

So maybe we keep this as a default but also take a new prop called eventName to build this string if possible?

I added a new prop and added it as required. I think is better if we make sure all the buttons have unique button names.

myasonik · 2021-02-02T21:42:54Z

src/components/notification/notification_event_meta.tsx

+            button={
+              <EuiI18n
+                token="euiNotificationEventMeta.contextMenuButton"
+                default="Open menu">


We don't need to specify our action here because we have aria-expanded in place. (If we did want to specify the action anyways, we'd need to toggle it to "Close menu" anyways.)

Though, because we can assume this same button will be rendered many times on one page, we should avoid having the same name for it.

If we accept a new prop called eventName we could build a string like: Menu for {eventName}

I built the string as you mentioned Menu for {eventName}.

myasonik · 2021-02-02T21:46:30Z

src/components/notification/notification_event_meta.tsx

+          />
+        )}
+
+        {iconType && <EuiIcon type={iconType} />}


Because we don't know if this icon will be purely decorative or provide contextual info, we also need a way to define an aria-label for this icon.

For example, if the cloud icon is going to be rendered here, we'd want to pass in Cloud... Unless we had a tag directly following it which already was labelled with Cloud in which case the icon is purely decorative.

Anyway, that will be up to the consuming app to decide but EUI needs to at least provide the opportunity.

I added a new prop called iconAriaLabel (it's optional). In case no iconAriaLabel is passed I'm assuming the icon is purely decorative so it receives an aria-hidden: true.

kibanamachine · 2021-02-03T14:04:58Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

elizabetdev · 2021-02-03T17:51:38Z

Hi again 👋🏽 ,

@cchaos I added the playground for the exported component: EuiNotificationEventReadButton. Also, I ensured the overflow menu stays in the top right.

@myasonik, I improved the a11y based on your review.

cchaos · 2021-02-03T18:52:44Z

Jenkins, test this

kibanamachine · 2021-02-03T19:17:06Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

myasonik

a11y looks good!

cchaos

Design and design code LGTM! 💯

…nu items

chandlerprall

Two small suggestions, and a small PR for unit tests elizabetdev#13

src/components/notification/notification_event_read_button.tsx

src/components/notification/notification_event_meta.tsx

EuiNotificationEventMeta unit tests update

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

kibanamachine · 2021-02-10T23:06:14Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4433/

chandlerprall

LGTM! Pulled & tested locally

mshustov · 2021-02-15T15:13:06Z

src/components/notification/notification_event_meta.tsx

+  /**
+   * Type of severity (e.g. "Critical", "Warning", etc..). Shows as a text after the `type` following the format "Alert: Critical".
+   */
+  severity?: string;


Should this severity level be translated according to the user locale? If so, I'd suggest defining a list of supported severity levels: normal, warning, critical.

The way that I was seeing this, is that this should be decided in the consuming app. The consuming app can translate it and passed as a string. One type of notification can have severities like "critical", "warning". And other types can have "high", "low".
But if you think it makes more sense I can change it to your suggested list: "normal", "warning", "critical".

Sorry, I didn't know what is better to place the question. It makes sense to keep Kibana specific logic outside of EUI. Kibana can define any severity level and implement any internal mapping to badgeColor.
Does it make sense to you?

Yes, It makes sense.

mshustov · 2021-02-15T15:42:26Z

src/components/notification/notification_event_meta.tsx

+  /**
+   * Accepts either our palette colors (primary, secondary ..etc) or a hex value `#FFFFFF`, `#000`.
+   */
+  badgeColor?: EuiBadgeProps['color'];


badgeColor should be derived from severity level?

It was the first idea. But because we didn't have a list it was difficult to predict the color. So it should be the consumer saying what is the color and create a logic with the severity level. But if we decide to change to a list like "normal", "warning", and "critical" we can derive the color from the severity level.

I'm going to create a list of "Left todo" in #4513. And then we can improve a few things according to feedback.

mshustov · 2021-02-15T15:46:06Z

src/components/notification/notification_event_read_button.tsx

+  /**
+   * A unique, human-friendly name for the event to be used in aria attributes (e.g. "alert-critical-01", "cloud-no-severity-12", etc..).
+   */
+  eventName: string;


so it consists of type-severity-number? It's okay if it's different on every render? In this case, we can implement a unique number as a global counter.

* [Feature Notification] Add EuiNotificationEventMeta (#4433) * Add notification event meta * WIP * adding i18n * Fixing axe error in docs example * Adding context menus items * contetx menu item anchor position * Adding tests * Update src/components/notification/notification_event_meta.tsx Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Improving code based on PR review * More PR review * Better description * Update src/components/notification/notification_event_meta.tsx Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_events/notification_events.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Badge 100% width * ES lint * Read and unread states * Make sure overflow menu stays in the top right * Improving a11y * Adding playground for event read button * Removing EuiNotificationEventMeta from exported on index * Add a behavior unit test, update context menu test to snapshot the menu items * Update src/components/notification/notification_event_meta.tsx Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * Update src/components/notification/notification_event_read_button.tsx Co-authored-by: Chandler Prall <chandler.prall@gmail.com> Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * [Feature Notification] Adding animation prop to EuiHeaderSectionItemButton (#4424) * Adding animation and hasBackground props to EuiHeaderSectionItemButton * A few improvements * Docs props * Removing hasBackground prop * Using JS do hide and show on xs breakpoints * Adding a snippet * refactor animation prop into a ref method * Snapshots. Trigger animation comments. * Deleting unused css animation * triggerAnimation explanation * Cleaned up some logic and created a docs example specifically for the notifications * re-enable the animation test * Update src-docs/src/views/header/header_animate.js Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * Update src-docs/src/views/header/header_example.js Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * Update src-docs/src/views/header/header_alert.js Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * Update src-docs/src/views/header/header_alert.js Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * Update src-docs/src/views/header/header_animate.js Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * Update src-docs/src/views/header/header_example.js Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * renaming animate methods to euiAnimate * euiAnimate into header example * simplifying onClick closeFlyout * Update src-docs/src/views/header/header_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> Co-authored-by: Chandler Prall <chandler.prall@gmail.com> Co-authored-by: cchaos <caroline.horn@elastic.co> Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * [Feature notification] Add EuiNotificationEvent (#4531) * Notification events * Looping through EuiNotificationEvent * Adding props table and more methods * wip * Renaming event notifications to event messages * onClickNoNewsTitles * Toggle mark as read/unread * Initial documentation * Improving examples * Fixing TS error * Refactored EuiEventNotification context menu item creation * improving data * Notifications data * Primary example buttons * Reset data * More docs * snippets * Adding tests * Better messages explanation * Better snippets * Small fixes * Missing prop explanation * Adding heading level * Adding flexible comp section * More a11y * Update src/components/notification/notification_event_messages.tsx Co-authored-by: Michail Yasonik <michail@yasonik.com> * Button messages ay11 * test * Better descriptions * Tests * Adding icon arial label into notifications feed example * i18n token * i18n token * i18 token again * primaryAction description * More description * Update src/components/notification/notification_event_messages.tsx Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src/components/notification/notification_event_messages.tsx Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Addressing PR review * More improvements * Adding comment * Excluding `eventName` at top level and changing title vs aria-label on read button * Fix title * Fixing tests and removing evenName from docs usage * Fixing icon margin * More tests and small fixes * Moving read button from event meta comp to event comp * Split primaryAction and primaryActionProps * Exposing EuiPrimaryActionProps in props table * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/notification_event/notification_event_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Messages length and more * Fixing data test subj * Renaming tsx examples to js Co-authored-by: Chandler Prall <chandler.prall@gmail.com> Co-authored-by: Michail Yasonik <michail@yasonik.com> Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> Co-authored-by: cchaos <caroline.horn@elastic.co> * Adding CL * Fixing CL a11y issue * Reverse logic in the ternary operator * Removed unecessary optional types in EuiNotificationEventMeta Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> Co-authored-by: Chandler Prall <chandler.prall@gmail.com> Co-authored-by: cchaos <caroline.horn@elastic.co> Co-authored-by: Michail Yasonik <michail@yasonik.com>

elizabetdev added 2 commits January 20, 2021 16:55

Add notification event meta

0a30dd8

WIP

f6d2943

elizabetdev changed the title ~~[Feature notification] Add notification event meta~~ [Feature Notification] Add notification event meta Jan 20, 2021

elizabetdev changed the title ~~[Feature Notification] Add notification event meta~~ [Feature Notification] Add EuiNotificationEventMeta Jan 20, 2021

elizabetdev force-pushed the notification-event-meta branch from fbe7ccd to f6d2943 Compare January 20, 2021 19:00

adding i18n

4110ea8

Fixing axe error in docs example

6be10cc

Adding context menus items

3e87679

elizabetdev added 2 commits January 25, 2021 16:45

contetx menu item anchor position

ffb70de

Adding tests

33283c1

elizabetdev marked this pull request as ready for review January 26, 2021 11:25

elizabetdev requested review from cchaos and chandlerprall January 26, 2021 11:25

elizabetdev commented Jan 26, 2021

View reviewed changes

src/components/notification/notification_event_meta.tsx Outdated Show resolved Hide resolved

cchaos suggested changes Jan 26, 2021

View reviewed changes

elizabetdev and others added 2 commits January 27, 2021 11:08

Update src/components/notification/notification_event_meta.tsx

f52b1f1

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

Improving code based on PR review

1d72b7f

More PR review

d87101e

elizabetdev commented Jan 27, 2021

View reviewed changes

src/components/notification/notification_event_meta.tsx Outdated Show resolved Hide resolved

Better description

7416eb7

elizabetdev requested a review from cchaos January 29, 2021 17:30

cchaos reviewed Feb 2, 2021

View reviewed changes

src/components/notification/_notification_event_meta.scss Outdated Show resolved Hide resolved

myasonik suggested changes Feb 2, 2021

View reviewed changes

elizabetdev added 2 commits February 3, 2021 12:05

Make sure overflow menu stays in the top right

183bd04

Improving a11y

2b3938b

elizabetdev added 2 commits February 3, 2021 17:24

Adding playground for event read button

798e654

Removing EuiNotificationEventMeta from exported on index

5dfe537

elizabetdev requested review from cchaos and myasonik February 3, 2021 17:52

myasonik approved these changes Feb 3, 2021

View reviewed changes

cchaos approved these changes Feb 3, 2021

View reviewed changes

Add a behavior unit test, update context menu test to snapshot the me…

4d0c431

…nu items

chandlerprall suggested changes Feb 10, 2021

View reviewed changes

src/components/notification/notification_event_read_button.tsx Outdated Show resolved Hide resolved

src/components/notification/notification_event_meta.tsx Outdated Show resolved Hide resolved

elizabetdev and others added 3 commits February 10, 2021 22:41

Merge pull request #13 from chandlerprall/notification-event-meta-test

17eb105

EuiNotificationEventMeta unit tests update

Update src/components/notification/notification_event_meta.tsx

d0692f4

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

Update src/components/notification/notification_event_read_button.tsx

70c26a7

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

elizabetdev requested a review from chandlerprall February 11, 2021 11:33

chandlerprall approved these changes Feb 11, 2021

View reviewed changes

elizabetdev merged commit 6a1058c into elastic:feature/notification Feb 11, 2021

elizabetdev mentioned this pull request Feb 11, 2021

[Feature branch] Notification #4513

Merged

10 tasks

mshustov reviewed Feb 15, 2021

View reviewed changes

Conversation

elizabetdev commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Jan 20, 2021

Uh oh!

kibanamachine commented Jan 20, 2021

Uh oh!

kibanamachine commented Jan 21, 2021

Uh oh!

kibanamachine commented Jan 21, 2021

Uh oh!

kibanamachine commented Jan 25, 2021

Uh oh!

kibanamachine commented Jan 25, 2021

Uh oh!

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Jan 27, 2021

Uh oh!

Uh oh!

elizabetdev commented Jan 27, 2021

Uh oh!

kibanamachine commented Jan 27, 2021

Uh oh!

kibanamachine commented Jan 27, 2021

Uh oh!

elizabetdev commented Jan 29, 2021

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 3, 2021

Uh oh!

elizabetdev commented Feb 3, 2021

Uh oh!

cchaos commented Feb 3, 2021

Uh oh!

kibanamachine commented Feb 3, 2021

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elizabetdev commented Jan 20, 2021 •

edited

Loading

mshustov Feb 15, 2021 •

edited

Loading

elizabetdev Feb 15, 2021 •

edited

Loading