Skip to content
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

[EuiComment] Revert username prop to accept a ReactNode #6071

Merged
merged 15 commits into from
Jul 26, 2022

Conversation

elizabetdev
Copy link
Contributor

@elizabetdev elizabetdev commented Jul 22, 2022

Summary

This PR reverts the EuiComment's username prop to accept a ReactNode instead of a string.

Not having the username as a string makes it impossible to have a timelineIcon defaulting to the username's initials. So the timelineAvatar now defaults to an avatar with a userAvatar icon.

username-prop@2x

Updated "A comment system" example

This example reflects more on the current implementation for Cases:

Screenshot 2022-07-22 at 13 13 32

Consumers can now pass a ReactNode to show the username with a tooltip and or avatar. As they doing in Cases

username-components@2x

aria-label for each EuiCommment.timelineIcon

One of the reasons I was trying to have the username as a string is that I could pass the username as a name to the timelineAvatar Having the username reverted to a ReactNode turns this impossible.

For this reason, I created a new prop called timelineAvatarAriaLabel that should be passed for each EuiCommment when a timelineAvatar is passed as an IconType:

Screenshot 2022-07-26 at 13 41 52

A sentence was added to the callout to reflect the above changes:

Screenshot 2022-07-26 at 14 29 18

Checklist

  • Checked 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 any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • [ ] Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elizabetdev elizabetdev marked this pull request as ready for review July 25, 2022 10:31
@elizabetdev elizabetdev requested review from cee-chen and 1Copenut July 25, 2022 10:31
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

LGTM! I retested in Safari and Firefox using VoiceOver. Both announced the containing div as an image and provided the expected aria-label. The nested SVG is being ignored properly by the Chrome accessibility overlay, and axe reports no violations.

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

LGTM, a few optional comments and thoughts but nothing blocking

src/components/comment_list/comment_timeline.tsx Outdated Show resolved Hide resolved
src/components/comment_list/comment_timeline.tsx Outdated Show resolved Hide resolved
src/components/comment_list/comment_timeline.tsx Outdated Show resolved Hide resolved
src/components/comment_list/comment_timeline.tsx Outdated Show resolved Hide resolved
elizabetdev and others added 3 commits July 25, 2022 20:07
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elizabetdev elizabetdev enabled auto-merge (squash) July 26, 2022 12:51
@kibanamachine
Copy link

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

@elizabetdev elizabetdev merged commit 03897b7 into elastic:main Jul 26, 2022
kibanamachine added a commit to cuff-links/kibana that referenced this pull request Aug 12, 2022
* Upgrade to v62.0.3

* Update EUI i18n tokens

* Update html string snapshots

- Emotion CSS hash changed

* [EuiIcon] Update instances of `keyboardShortcut` icons to `keyboard`

* [EuiErrorBoundary] Update snapshots from Emotion conversion

* [EuiImage] Update snapshots, tests, and CSS to account for Emotion conversion

* [EuiImage][RTL] Fix test failures caused by EuiImage changes

* [EuiCommentList] Deprecate EuiCommentProps.type

* [EuiCommentList] Rename `timelineIcon` prop to `timelineAvatar`

- see elastic/eui#6071

* [EuiCommentList] Fix selectors deprecated by Emotion conversion

* [EuiPopover][EuiCommentEvent][Enzyme] Fix mounted test failures caused by Emotion conversions

- Mounting displays the Emotion wrapper with the data-test-subj on them - we need to specify the output div renders in order for text assertions to be correct

* [EuiPopover] Deprecate `initialFocus={false}` as an option

see elastic/eui#6044

* [EuiPopover] Rename `display=inlineBlock` to `inline-block`

- see elastic/eui#5977

* [EuiPopover] Update snapshots from Emotion conversion

* [EuiPopover] Replace deprecated `.euiPopover__panel-isOpen` class with new `[data-popover-open]` attribute

* [EuiPopover][RTL] Fix test failures caused by not waiting for EuiPopover animation/transition

* Skip failing a11y tests

- test w/ similar error already skipped in another test above
- requires closing the popover for next test to pass
- not sure why delete action is no longer available

* Fix failing Security Cypress tests

* Attempt to squash flaky FTR tests around Add Filter popover

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Jonathan Budzenski <jon@elastic.co>
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
* Upgrade to v62.0.3

* Update EUI i18n tokens

* Update html string snapshots

- Emotion CSS hash changed

* [EuiIcon] Update instances of `keyboardShortcut` icons to `keyboard`

* [EuiErrorBoundary] Update snapshots from Emotion conversion

* [EuiImage] Update snapshots, tests, and CSS to account for Emotion conversion

* [EuiImage][RTL] Fix test failures caused by EuiImage changes

* [EuiCommentList] Deprecate EuiCommentProps.type

* [EuiCommentList] Rename `timelineIcon` prop to `timelineAvatar`

- see elastic/eui#6071

* [EuiCommentList] Fix selectors deprecated by Emotion conversion

* [EuiPopover][EuiCommentEvent][Enzyme] Fix mounted test failures caused by Emotion conversions

- Mounting displays the Emotion wrapper with the data-test-subj on them - we need to specify the output div renders in order for text assertions to be correct

* [EuiPopover] Deprecate `initialFocus={false}` as an option

see elastic/eui#6044

* [EuiPopover] Rename `display=inlineBlock` to `inline-block`

- see elastic/eui#5977

* [EuiPopover] Update snapshots from Emotion conversion

* [EuiPopover] Replace deprecated `.euiPopover__panel-isOpen` class with new `[data-popover-open]` attribute

* [EuiPopover][RTL] Fix test failures caused by not waiting for EuiPopover animation/transition

* Skip failing a11y tests

- test w/ similar error already skipped in another test above
- requires closing the popover for next test to pass
- not sure why delete action is no longer available

* Fix failing Security Cypress tests

* Attempt to squash flaky FTR tests around Add Filter popover

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Jonathan Budzenski <jon@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants