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

fixed a11y Logo Issue and added Translations #5722

Merged
merged 4 commits into from
Feb 1, 2024
Merged

fixed a11y Logo Issue and added Translations #5722

merged 4 commits into from
Feb 1, 2024

Conversation

Molochem
Copy link
Contributor

fixes #5720

Fixed the Alt-Title of the Logo to say "Back to Homepage" in various Languages for a11y and Screenreader conformity

@netlify Netlify
Copy link

netlify bot commented Jan 31, 2024 •
edited
Loading

✅ Deploy Preview for volto canceled.

Name Link
🔨 Latest commit a4a654b
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/65ba6dc93e4b0200085fa404

@netlify Netlify
Copy link

netlify bot commented Jan 31, 2024 •
edited
Loading

✅ Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit a4a654b
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/65ba6dc981f8730008a95634

@Molochem Molochem requested a review from sneridagh January 31, 2024 16:28
davisagli
davisagli approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

Looks good to me.

In the future, you don't have to provide all translations for a new message; you can leave that for native speakers of the languages to fill in later.

@davisagli davisagli merged commit 4e741b9 into main Feb 1, 2024
63 checks passed
@davisagli davisagli deleted the fixA11yLogoIssue branch February 1, 2024 03:06
@erral
Copy link
Member

erral commented Feb 1, 2024

I would say that "back to home page" is not a good alternative text for the logo right?

I understand that perhaps we need to signal that, but perhaps it is better to signal that in an aria-whatever attribute? Or perhaps add the said alternative text to the link instead of to the logo?

@stevepiercy
Copy link
Collaborator

@erral is correct. A logo represents a brand or identity, not an action.

Also it is "home page", not "homepage".

And what if the user enters a site not on the home page? They would not "go back to home page", but would "visit home page".

@ichim-david
Copy link
Member

ichim-david commented Feb 1, 2024 •
edited
Loading

@erral is correct. A logo represents a brand or identity, not an action.

Also it is "home page", not "homepage".

And what if the user enters a site not on the home page? They would not "go back to home page", but would "visit home page".

@stevepiercy Maybe it should simply be "Go to home page" this way there is no visit or back involved.
@davisagli @sneridagh this is a breaking change as it modifies the markup and what is shown to the user when hovering or when using a screen reader, if my change to remove inline style from was deemed as breaking From BlockChooserSearch.jsx
34e77db
which I've reverted so that the changes could have been merged to 17.x.x than so should this change be deemed as breaking.
Given this feedback from others like @erral and @stevepiercy maybe we need to further discuss in the original or another issue if we need to change it to something else.

sneridagh added a commit that referenced this pull request Feb 1, 2024
@sneridagh
Merge branch 'main' into react18
0110e91 
* main:
  Block search engines from indexing content on Netlify preview builds (#5725)
  fixed a11y Logo Issue and added Translations (#5722)
  Allow editor to edit metadata name during bulk or single upload (#5560)
  5439 clarify defaultAddonName option and mention to the generator readme the creation of the theme add-on (#5709)
  Remove turbo from monorepo commands until it's really necessary (#5715)
  bugfix: wrong conditional proprieties on ObjectBrowser (#4190)
  Added aria-live='polite' (#5639)
  Release 18.0.0-alpha.9
  Release @plone/scripts 3.3.2
  fix: handle addons that have not been migrated to the new structure o… (#5704)
  Update Semantic UI React to version 2.1.5 (#5632)
  Removed unmaintained and unused razzle-plugin-bundle-analyze (#5671)
  3716 - improve validation of id widget (#4686)
  Fixed redirect of https://tanstack.com/query/v4/docs/react/guides/ssr… (#5700)
  Release 18.0.0-alpha.8
  Release @plone/volto 18.0.0-alpha.7 (not pushed to main)
  Release @plone/slate 18.0.0-alpha.6
  Release @plone/registry 1.2.1 (not pushed to repo)
@davisagli
Copy link
Member

@erral @stevepiercy @ichim-david @sneridagh Sorry, it seems I was too quick to merge this. We can revert it and discuss further. I'll do that later today.

davisagli added a commit that referenced this pull request Feb 1, 2024
@davisagli
Revert "fixed a11y Logo Issue and added Translations (#5722)"
5242643 
This reverts commit 4e741b9.
@davisagli
Copy link
Member

@erral @stevepiercy @ichim-david @sneridagh @Molochem Reverted.

This was referenced Feb 1, 2024
Closed
Merged
@Molochem Molochem restored the fixA11yLogoIssue branch February 20, 2024 13:59
@davisagli davisagli mentioned this pull request Sep 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davisagli davisagli davisagli approved these changes

@sneridagh sneridagh Awaiting requested review from sneridagh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Logo alt-Title not a11y/Screenreader compliant
5 participants
@Molochem @erral @stevepiercy @ichim-david @davisagli