feat(Avatar): add avatar component #1510
Conversation
Codecov Report
@@ Coverage Diff @@
## next #1510 +/- ##
=======================================
Coverage ? 91.91%
=======================================
Files ? 280
Lines ? 4216
Branches ? 767
=======================================
Hits ? 3875
Misses ? 316
Partials ? 25
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
size-limit report 📦
|
|
Pending:
|
booc0mtaco
force-pushed
the
eds-824-create-avatar-component
branch
3 times, most recently
from
56e7135
to
91dd309
Compare
booc0mtaco
force-pushed
the
eds-824-create-avatar-component
branch
2 times, most recently
from
faf9d46
to
ca1d56c
Compare
booc0mtaco
force-pushed
the
eds-824-create-avatar-component
branch
2 times, most recently
from
8074a90
to
4904c19
Compare
src/components/Avatar/Avatar.tsx
Outdated
| `Avatar for ${!user && 'unknown'} user ${user?.fullName || ''}`; | ||
|
|
||
| // when not a button, add some props to allow focus states | ||
| const a11yProps = TagName === 'button' ? {} : { role: 'button', tabIndex: 0 }; |
There was a problem hiding this comment.
What's the scenario for using a div vs button?
Wondering if this should either be:
- Always a div, since I don't see an
onClickhandler or other interaction? - Always a button, since we're giving it a button role anyway?
There was a problem hiding this comment.
There are these two cases:
- an avatar can appear as an inert component in things like cards, or profiles (where there is no interaction)
- as an actual button (imagine a profile menu, where clicking the avatar shows a
Menu's content below)
Now that I think about it, in the former case it wouldn't need such a role or tab index b/c there's nothing to do ... its essentially an image then. This was initially to ensure focus state was working but should be removed now
There was a problem hiding this comment.
@ahuth it may be true that it should never be a button, and cases where it is clickable would be wrapped by a button? maybe I should just take this part out for now, and keep it simple
There was a problem hiding this comment.
I personally think avatar should beinert, and if interactive functionality is needed, it will be wrapped by an appropriate element.
However, this does complicate focus states, but using will assume to better fit target size requirements
There was a problem hiding this comment.
Not sure of the right answer, but maybe it's best to start with the simpler solution. Can add in a button later if we think it's better.
There was a problem hiding this comment.
@jinlee93 @ahuth Agreed. I thought about it some more , and i think what's happening here is actually more like "an avatar can be wrapped by an 'invisible' button" where the button itself has the focus ring instead of the avatar... I'm gonna take this out and let the avatar be more image-like.
There are actually 1-2 other things behaving like this in EDS already, and I've seen this variant in other design systems i've worked on, so i will try some things out in a follow-up
booc0mtaco
force-pushed
the
eds-824-create-avatar-component
branch
2 times, most recently
from
ad559ed
to
ab9fcab
Compare
- implement core component - add in variants, sizes, and shapes - add in new avatar icon to spritesheet - specify default aria label - add tests and stories - add test for focus state
booc0mtaco
force-pushed
the
eds-824-create-avatar-component
branch
from
ab9fcab
to
a2b97ef
Compare
## [10.0.0](v9.1.0...v10.0.0) (2023-03-02) ### ⚠ BREAKING CHANGES * remove data-bootstrap-override EDS-850 ### Features * **Avatar:** add avatar component ([#1510](#1510)) ([bc21f85](bc21f85)) * **slider:** create slider ([#1503](#1503)) ([e7ced34](e7ced34)) * **TextareaField:** add TextArea base component and TextareaField ([#1493](#1493)) ([f2ba31d](f2ba31d)) ### Bug Fixes * remove data-bootstrap-override EDS-850 ([3b5d59b](3b5d59b)) * remove old, outdated, unnecessary snapshot ([fb63a34](fb63a34)) * **Select:** sync label design with form fields ([#1506](#1506)) ([efe9330](efe9330)) * update deps ([9a80e7f](9a80e7f)) * upgrade axe-core from 4.4.3 to 4.6.3 ([af3f9c5](af3f9c5))
Summary:
Test Plan:
edu-stackortrajectas a sanity check if changes affect build or deploy, or are breaking, such as token changes, widely used component updates, hooks changes, and major dependency upgrades.