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

Fix/missing alt for profile image #12082

Merged

Conversation

mikeslinkman
Copy link
Contributor

Description

Adds the alt attribute for the avatar of a user based on the users name.

Visual changes

No visual changes

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@zepfietje
Copy link
Member

Wouldn't it be ok to just use the user name without any prefix?
Also, this should probably be added to the tenant avatar in the tenant menu too.

@zepfietje zepfietje added the enhancement New feature or request label Mar 29, 2024
@zepfietje zepfietje added this to the v3 milestone Mar 29, 2024
@mikeslinkman
Copy link
Contributor Author

mikeslinkman commented Mar 29, 2024

Wouldn't it be ok to just use the user name without any prefix? Also, this should probably be added to the tenant avatar in the tenant menu too.

It would be an improvement compared to the current situation, but we would leave the user "guessing" what the context for the image is. Ideally we would add the additional context, and I would be willing to add more translations by using google translate but first wanted to get a initial response. I'll add it for the tenant variant as well once we've settled on the way we will handle the translations (if we handle these)

** Edit **

Just discussed internally with our Accessibility Engineer and he told me the best would be to do it like this:

Avatar of [Name] // EN
Avatar van [Name] // NL

When we add alt text in these situations for our own projects (also for logo purposes of the company) we normally do it like this [Company Name] logo compared to [Comapny Name] just adds a little more context for people.

@zepfietje
Copy link
Member

zepfietje commented Mar 29, 2024

Thanks for clarifying, @mikeslinkman.
I agree, though we should discuss if avatar / profile picture / profile photo or something else fits best in this case. Kinda tricky since it may depend on the actual application.

Also, after this PR, when we add it to tenant avatars too, we should also update packages/panels/resources/views/components/logo.blade.php to use a "logo" suffix.

@mikeslinkman
Copy link
Contributor Author

Thanks for clarifying, @mikeslinkman. I agree, though we should discuss if avatar / profile picture / profile photo or something else fits best in this case. Kinda tricky since it may depend on the actual application.

Also, after this PR, when we add it to tenant avatars too, we should also update packages/panels/resources/views/components/logo.blade.php to use a "logo" suffix.

Yes, I can totally add it over there as well. I'm open to any of those but I have a slight preference for using 'Avatar' since this is also the best description when it is not an actual photo but instead shows the initials of the authenticated user. But also happy to explore other options

@danharrin
Copy link
Member

+1 for avatar, its the terminology we use internally anyway

@danharrin danharrin added the a11y label Mar 29, 2024
@danharrin danharrin self-assigned this Mar 29, 2024
Copy link
Member

@danharrin danharrin left a comment

Choose a reason for hiding this comment

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

While this is open, please also apply this so the tenant avatar, which can use the same translation string

@zepfietje
Copy link
Member

While this is open, please also apply this so the tenant avatar, which can use the same translation string

And add the "logo" suffix/prefix to the brand logo component alt.

@danharrin
Copy link
Member

For that one, the translation string can be layout.logo.alt I think

@danharrin danharrin merged commit df80499 into filamentphp:3.x Apr 1, 2024
10 checks passed
@danharrin
Copy link
Member

Thanks @mikeslinkman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants