[EuiComment] Revert username prop to accept a ReactNode#6071
Merged
elizabetdev merged 15 commits intoelastic:mainfrom Jul 26, 2022
Merged
[EuiComment] Revert username prop to accept a ReactNode#6071elizabetdev merged 15 commits intoelastic:mainfrom
username prop to accept a ReactNode#6071elizabetdev merged 15 commits intoelastic:mainfrom
Conversation
elizabetdev
commented
Jul 22, 2022
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6071/ |
cee-chen
reviewed
Jul 22, 2022
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6071/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6071/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6071/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6071/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6071/ |
1Copenut
approved these changes
Jul 25, 2022
Contributor
1Copenut
left a comment
There was a problem hiding this comment.
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.
cee-chen
approved these changes
Jul 25, 2022
Contributor
cee-chen
left a comment
There was a problem hiding this comment.
LGTM, a few optional comments and thoughts but nothing blocking
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>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6071/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6071/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6071/ |
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Summary
This PR reverts the EuiComment's
usernameprop to accept aReactNodeinstead of astring.Not having the
usernameas a string makes it impossible to have atimelineIcondefaulting to theusername's initials. So thetimelineAvatarnow defaults to an avatar with auserAvataricon.Updated "A comment system" example
This example reflects more on the current implementation for Cases:
Consumers can now pass a
ReactNodeto show the username with a tooltip and or avatar. As they doing in Casesaria-labelfor eachEuiCommment.timelineIconOne of the reasons I was trying to have the
usernameas astringis that I could pass the username as a name to thetimelineAvatarHaving the username reverted to aReactNodeturns this impossible.For this reason, I created a new prop called
timelineAvatarAriaLabelthat should be passed for eachEuiCommmentwhen atimelineAvataris passed as anIconType:A sentence was added to the callout to reflect the above changes:
Checklist
[ ] Checked in mobile[ ] Checked Code Sandbox works for any docs examples[ ] Updated the Figma library counterpart