Skip to content

[Feature notification] Add EuiNotificationEvent#4531

Merged
elizabetdev merged 67 commits intoelastic:feature/notificationfrom
elizabetdev:notification-events-content
Mar 9, 2021
Merged

[Feature notification] Add EuiNotificationEvent#4531
elizabetdev merged 67 commits intoelastic:feature/notificationfrom
elizabetdev:notification-events-content

Conversation

@elizabetdev
Copy link
Contributor

@elizabetdev elizabetdev commented Feb 16, 2021

Summary

This PR adds a EuiNotificationEvent and all the documentation.

Screenshot 2021-02-25 at 14 36 07

Screenshot 2021-02-25 at 14 34 58

Notifications feed

This PR also adds a section in the docs to show how to render multiple EuiNotificationEvent.

Screenshot 2021-02-25 at 14 42 40

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • [ ] Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • [ ] A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

…s-content-context-menu-items

Notification events content context menu items
@elizabetdev elizabetdev changed the title [Feature notification] Add EuiNotificationEvents [Feature notification] Add EuiNotificationEvent Feb 24, 2021
@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor Author

Based on the PR reviews:

  • Tests and docs usage were updated to reflect the EventName change.
  • Updated the more messages button. Now it says + 2 more and the link is using the primary color. If there are only 2 messages the button doesn't appear. I think it was weird to add just one message to the accordion. The icon is now properly aligned with the bottom content.

Screenshot 2021-03-04 at 15 49 25

  • Fixed the demo to make it responsive (badge truncates) and to reflect the meta props that are now spread out:

Screenshot 2021-03-04 at 15 51 03

  • No more playground for the EuiNotificationEventReadButton component and it is no longer exported.
  • I did a few structural changes. The EuiNotificationEventReadButton is no longer part of the meta component.
  • I also tried different usage scenarios. With and without the read button and the context menu.

tests@2x

  • Split the primaryAction prop into primaryAction and primaryActionProps

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Awesome, thank you for those updates @miukimiu !

I'm just seeing a couple more nit-picky design things.

  1. I think because you've pulled the button out of the meta component, which established the overall min height the rest of the meta stuff aligned well vertically. Now not so much. Maybe that container just needs a min-height of 24px (the same as the size of the button).

Screen Shot 2021-03-04 at 14 02 28 PM

  1. The only thing about showing the accordion only if there are more than 2 messages is the inconsistency of showing only 1 message when there are 3+. It's much better to have a consistent understanding that only the first message is ever shown, even if it says "+1 more".

The rest of my comments are mainly grammatical. But I really love that the event itself has all the required props at the top level now. It'll be so much easier to implement.

elizabetdev and others added 7 commits March 5, 2021 11:04
…le.js

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

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

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

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

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

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

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

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elizabetdev elizabetdev requested a review from cchaos March 8, 2021 12:52
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Thank you for making all those changes. The docs are really helpful in explaining the correct usages of this component. 🎉

@elizabetdev elizabetdev merged commit 917d59a into elastic:feature/notification Mar 9, 2021
@elizabetdev elizabetdev mentioned this pull request Mar 9, 2021
10 tasks
elizabetdev added a commit that referenced this pull request Mar 16, 2021
* [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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants