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.

Sign up for GitHub

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

Jump to bottom

Add id prop to ListItemIcon to prevent v-binding to the Avatar #1833

Merged
merged 1 commit into from
Apr 12, 2021
Merged

Add id prop to ListItemIcon to prevent v-binding to the Avatar #1833

merged 1 commit into from
Apr 12, 2021

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Apr 9, 2021

Because we v-bind="$attrs" to the avatar component, declaring id pass it to the $props and make sure we are not having a duplicate ID on the page
image

@skjnldsv skjnldsv added bug Something isn't working 3. to review Waiting for reviews feature: avatar Related to the avatar component accessibility Making sure we design for the widest range of people possible, including those who have disabilities feature: list-item-icon Related to the list-item-icon component labels Apr 9, 2021
@skjnldsv skjnldsv self-assigned this Apr 9, 2021
@raimund-schluessler
Copy link
Contributor

On a side note: I find the naming of this component confusing. It should rather be ListItemAvatar, I believe. 😉🙈

@raimund-schluessler raimund-schluessler self-requested a review April 9, 2021 09:40
@skjnldsv
Add id prop to ListItemIcon to prevent v-binding to the Avatar
63f2ba0 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the fix/listitemicon-inherit-id-avatar branch from 20a08c6 to 63f2ba0 Compare April 12, 2021 07:25
@skjnldsv skjnldsv requested a review from jotoeri April 12, 2021 07:25
@skjnldsv
Copy link
Contributor Author

skjnldsv commented Apr 12, 2021 •
edited
Loading

On a side note: I find the naming of this component confusing. It should rather be ListItemAvatar, I believe.

Yeah, we have the same discussion with the incoming listItem component lol.
It have an icon or a menu at the end, that was my original naming intent, but the avatar is also relevant.
Feel free to open a pr for 4.0.0 😉

ChristophWurst
ChristophWurst approved these changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

I'll allow it

@raimund-schluessler raimund-schluessler merged commit 5bdd0a4 into master Apr 12, 2021
@raimund-schluessler raimund-schluessler deleted the fix/listitemicon-inherit-id-avatar branch April 12, 2021 09:14
@skjnldsv skjnldsv mentioned this pull request Jun 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jotoeri jotoeri jotoeri approved these changes

@raimund-schluessler raimund-schluessler raimund-schluessler approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@korelstar korelstar Awaiting requested review from korelstar

@julien-nc julien-nc Awaiting requested review from julien-nc

Assignees

@skjnldsv skjnldsv

Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: avatar Related to the avatar component feature: list-item-icon Related to the list-item-icon component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skjnldsv @raimund-schluessler @ChristophWurst @jotoeri