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

[EuiAvatar] Add letter casing prop support and default to capitalizing initials #6739

Merged
merged 8 commits into from
May 1, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Apr 27, 2023

Summary

@dimadavid recently brought up that certain lowercase letters look vertically off in EuiAvatar, and proposed a workaround of always ensuring displayed initials are in uppercase.

Per designer feedback, we're defaulting to uppercase for all user avatars, but leaving spaces avatars without an opinionated text transform. Both avatar types can override these defaults via the new casing prop.

screencap

QA

General checklist

  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Added documentation
  • Added or updated jest and cypress tests
  • A changelog entry exists and is marked appropriately
    - [ ] Updated the Figma library counterpart Figma doesn't seem to account for lowercase letters

- [ ] Checked in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked Code Sandbox works for any docs examples
- [ ] Checked for breaking changes and labeled appropriately
- [ ] Checked for accessibility including keyboard-only and screenreader modes

@cee-chen cee-chen changed the title [EuiAvatar] Add letter casing prop support and default to capitaizing initials [EuiAvatar] Add letter casing prop support and default to capitalizing initials Apr 27, 2023
@kibanamachine
Copy link

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

@cee-chen
Copy link
Member Author

@mdefazio @andreadelrio - any chance either of you can sign off on this change (both the default change to text-transform: capitalize and allowing consumers to customize casing)?

@cee-chen cee-chen marked this pull request as ready for review April 27, 2023 21:28
@kibanamachine
Copy link

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

@andreadelrio
Copy link
Contributor

andreadelrio commented Apr 27, 2023

@mdefazio @andreadelrio - any chance either of you can sign off on this change (both the default change to text-transform: capitalize and allowing consumers to customize casing)?

Based on our current EuiAvatar docs, which specifically display initials such as Ki and En, I opted instead to default to capitalize and to add a new casing prop to allow consumers to quickly and easily customize their use-cases as needed.

@cee-chen I'm not sure I'm following the reasoning behind not defaulting to uppercase but capitalizing instead. Can you please clarify?

Update: I guess I was assuming that most of the times the name would be a full name (i.e. first name + last name) but I see in our docs that the name could just be one word that's when capitalizing makes more sense. I would imagine that the full name scenario is a more frequent use case and therefore would default to uppercase.

@cee-chen
Copy link
Member Author

cee-chen commented Apr 27, 2023

TBH I'm totally good with defaulting to uppercase vs. capitalize if y'all are. We just had several demos that only had the first letter capitalized in prod:

so I assumed that was kind of the desired default standard of display. If we decide that defaulting to uppercase instead makes sense, I'm good with that since the new prop lets consumers override it if need be.

@andreadelrio
Copy link
Contributor

If we decide that defaulting to uppercase instead makes sense, I'm good with that since the new prop lets consumers override it if need be.

Amazing! Well my vote is to default to uppercase.

@cee-chen
Copy link
Member Author

Awesome, let's do it! Although before I rush off to make that change in code, any thoughts or objections @mdefazio?

@mdefazio
Copy link
Contributor

Update: I guess I was assuming that most of the times the name would be a full name (i.e. first name + last name) but I see in our docs that the name could just be one word that's when capitalizing makes more sense. I would imagine that the full name scenario is a more frequent use case and therefore would default to uppercase.

Assuming this change also effects the space variant?

When creating a space, I can choose to tweak the initials. The current functionality goes:

  • One word, avatar shows one letter (Uppercase)
  • Two wards, avatar shows two letters (both Uppercase)
  • I can tweak the initials for the space to be whatever I want...such as two lowercase (does not have to match the name).

Would this PR remove this functionality?
Are we sure no one wants a space name like dF or cc?

image

image

One other point to consider for the avatar...

image

vs

image

Not sure we'd want the first example to be forced to use HV

@cee-chen
Copy link
Member Author

cee-chen commented Apr 28, 2023

Just wanted to preface that I love these kinds of in-depth discussions and attention to detail. It is totally my jam. Y'all rock.

Assuming this change also effects the space variant?

We could specifically tweak the type="space" avatar to casing="none" (but type="user" avatars still get casing="uppercase").

I'm not sure that would satisfy @dimadavid's complaint about some lowercase letters feeling visually off, but I think we'll have to compromise on that and let user input take priority for spaces.

One other point to consider for the avatar [...] Not sure we'd want the first example to be forced to use HV

My 2c: I don't think it's the worst if we do. It's fairly consistent at least, and I don't know if there's a single perfect solution, just the ability for us to allow consumers to flexibly customize their display if needed. Andrea, any thoughts?

@andreadelrio
Copy link
Contributor

One other point to consider for the avatar [...] Not sure we'd want the first example to be forced to use HV

My 2c: I don't think it's the worst if we do. It's fairly consistent at least, and I don't know if there's a single perfect solution, just the ability for us to allow consumers to flexibly customize their display if needed. Andrea, any thoughts?

+1. I believe that having a last name starting with a lowercase letter is an edge case. While we aim to accommodate for those it's not always possible to find a universal solution. I think the casing prop should be enough to provide customization to consumers who care about that edge case.

@cee-chen
Copy link
Member Author

Alrighty, I've updated type="user" avatars to default to uppercase and type="spaces" avatars to default to no changes to casing (although of course consumers can still override both via the casing prop).

Any last thoughts or change requests? I'd love to hear 'em!

@kibanamachine
Copy link

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

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Cee!

@cee-chen cee-chen merged commit d1ca8b9 into elastic:main May 1, 2023
@cee-chen cee-chen deleted the avatar-uppercase branch May 1, 2023 16:01
@cee-chen cee-chen self-assigned this May 1, 2023
jbudz pushed a commit to elastic/kibana that referenced this pull request May 15, 2023
## Summary

`eui@77.2.2` ⏩ `eui@79.0.1`

🦴 The primary changes in this upgrade are around the deprecated
`EuiLoadingContent` being removed in favor of `EuiSkeletonText`.
- Most instances have been a [direct swap of
usage](327626a),
but [some replacements were a bit more
opinionated](e6ceb36)
as I saw them as potential to take advantage of `EuiSkeletonText`'s
syntactical sugar and screen reader announcements for when state
switches to loaded.

---

## [`79.0.1`](https://github.com/elastic/eui/tree/v79.0.1)

**Bug fixes**

- Fixed broken push `EuiFlyout` behavior
([#6764](elastic/eui#6764))


## [`79.0.0`](https://github.com/elastic/eui/tree/v79.0.0)

- Updated all `EuiSkeleton` components with new props that allow for
more control over screen reader live announcements:
`announceLoadingStatus`, `announceLoadedStatus`, and `ariaLiveProps`
([#6752](elastic/eui#6752))
- Improved keyboard accessibility in `EuiPageHeader` by ensuring the
right side menu items come into focus from left to right.
([#6753](elastic/eui#6753))

**Breaking changes**

- Removed deprecated `EuiLoadingContent`. Use the `EuiSkeleton`
components instead. ([#6754](elastic/eui#6754))


## [`78.0.0`](https://github.com/elastic/eui/tree/v78.0.0)

- Improved the contrast ratio of `EuiCheckbox`, `EuiRadio`, and
`EuiSwitch` in their unchecked states to meet WCAG AA guidelines.
([#6729](elastic/eui#6729))
- Added React Testing Library `*ByTestSubject` custom commands to
`within()`. RTL utilities can be imported from
`@elastic/eui/lib/test/rtl`.
([#6737](elastic/eui#6737))
- Updated `EuiAvatar` to support a new letter `casing` prop that allow
customizing text capitalization
([#6739](elastic/eui#6739))
- Updated `EuiFocusTrap` to support the `gapMode` prop configuration
(now defaults to `padding`)
([#6744](elastic/eui#6744))

**Bug fixes**

- Fixed inconsistency in `EuiSearchBar`'s AND/OR semantics between DSL
and query string generation
([#6717](elastic/eui#6717))
- Fixed `EuiFieldNumber`'s native browser validity detection causing
extra unnecessary rerenders
([#6741](elastic/eui#6741))
- Fixed the `scrollLock` property on `EuiFocusTrap` (and other
components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to
no longer block scrolling on nested portalled content, such as combobox
dropdowns ([#6744](elastic/eui#6744))

**Breaking changes**

- `EuiAvatar`s with the default `user` type will now default to
capitalizing all initials in uppercase
([#6739](elastic/eui#6739))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jasonrhodes pushed a commit to elastic/kibana that referenced this pull request May 17, 2023
## Summary

`eui@77.2.2` ⏩ `eui@79.0.1`

🦴 The primary changes in this upgrade are around the deprecated
`EuiLoadingContent` being removed in favor of `EuiSkeletonText`.
- Most instances have been a [direct swap of
usage](327626a),
but [some replacements were a bit more
opinionated](e6ceb36)
as I saw them as potential to take advantage of `EuiSkeletonText`'s
syntactical sugar and screen reader announcements for when state
switches to loaded.

---

## [`79.0.1`](https://github.com/elastic/eui/tree/v79.0.1)

**Bug fixes**

- Fixed broken push `EuiFlyout` behavior
([#6764](elastic/eui#6764))


## [`79.0.0`](https://github.com/elastic/eui/tree/v79.0.0)

- Updated all `EuiSkeleton` components with new props that allow for
more control over screen reader live announcements:
`announceLoadingStatus`, `announceLoadedStatus`, and `ariaLiveProps`
([#6752](elastic/eui#6752))
- Improved keyboard accessibility in `EuiPageHeader` by ensuring the
right side menu items come into focus from left to right.
([#6753](elastic/eui#6753))

**Breaking changes**

- Removed deprecated `EuiLoadingContent`. Use the `EuiSkeleton`
components instead. ([#6754](elastic/eui#6754))


## [`78.0.0`](https://github.com/elastic/eui/tree/v78.0.0)

- Improved the contrast ratio of `EuiCheckbox`, `EuiRadio`, and
`EuiSwitch` in their unchecked states to meet WCAG AA guidelines.
([#6729](elastic/eui#6729))
- Added React Testing Library `*ByTestSubject` custom commands to
`within()`. RTL utilities can be imported from
`@elastic/eui/lib/test/rtl`.
([#6737](elastic/eui#6737))
- Updated `EuiAvatar` to support a new letter `casing` prop that allow
customizing text capitalization
([#6739](elastic/eui#6739))
- Updated `EuiFocusTrap` to support the `gapMode` prop configuration
(now defaults to `padding`)
([#6744](elastic/eui#6744))

**Bug fixes**

- Fixed inconsistency in `EuiSearchBar`'s AND/OR semantics between DSL
and query string generation
([#6717](elastic/eui#6717))
- Fixed `EuiFieldNumber`'s native browser validity detection causing
extra unnecessary rerenders
([#6741](elastic/eui#6741))
- Fixed the `scrollLock` property on `EuiFocusTrap` (and other
components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to
no longer block scrolling on nested portalled content, such as combobox
dropdowns ([#6744](elastic/eui#6744))

**Breaking changes**

- `EuiAvatar`s with the default `user` type will now default to
capitalizing all initials in uppercase
([#6739](elastic/eui#6739))

---------

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

Successfully merging this pull request may close these issues.

4 participants