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

XWIKI-22484: The alternative for the home link on the logo does not mean anything #3510

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Sep 19, 2024

Jira URL

https://jira.xwiki.org/browse/XWIKI-22484

Changes

Description

  • Updated the companylogo template to add a proper text alternative
  • Added translations for those text alternatives

Screenshots & Video

After the changes proposed in this PR:
Screenshot from 2024-09-19 11-21-37

Executed Tests

None except manual tests (see screeshot above). Checked functional tests and it seems the selector for the element we modified is intact:

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • 16.4.X
    • 15.10.X

…ean anything

* Updated the companylogo template to add a proper text alternative
* Added translations for those text alternatives
@Sereza7 Sereza7 added backport stable-15.10.x Used for automatic backport to 15.10.x branch. backport stable-16.4.x labels Oct 10, 2024
Copy link
Contributor

@michitux michitux left a comment

Choose a reason for hiding this comment

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

Overall, this looks good, but as this is one of the first elements of the page, I think we could be a bit more careful regarding the wording (see individual comments).

@@ -46,7 +46,8 @@
#end
#end
<div id="companylogo">
<a href="$!xwiki.getURL($services.wiki.currentWikiDescriptor.mainPageReference)" title="Home" rel="home" #if(!$displayPageHeader)class="navbar-brand"#end>
<img src="$!logourl" alt="Wiki Logo"/>
<a href="$!xwiki.getURL($services.wiki.currentWikiDescriptor.mainPageReference)" rel="home" #if(!$displayPageHeader)class="navbar-brand"#end>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that removing the title from the link is good as it means that users won't get any hint anymore that the link points to the home page of the wiki when they're hovering the logo. I understand that the title attribute might not be enough for screen readers, but I think it would still be good to keep the title attribute, but maybe with the new translated text.

@@ -752,6 +752,9 @@ core.footer.modification=Last modified by {0} on {1}
## This label is only visible by AT users to help with understanding the meaning of the top section containing
## the page title and the info about the last modification of the page.
core.document.header.info.label=Page header
## These two text alternatives are only visible by AT users.
core.document.header.companylogo.anchor.text=Wiki home
core.document.header.companylogo.logo.text=Logo of the wiki
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering some things:

  1. Should we really use the name companylogo? I see that it is the ID of the container, but in my opinion it isn't really fitting as the wiki doesn't necessarily belong to a company. What about a shorter "logo" and then "image" instead of the "logo" for the second key?
  2. Should we really name the text "Wiki home", why don't we keep the shorter "Home" that was the title previously? Asides from being shorter, there is also the aspect that from the perspective of the user, this website might not really be a wiki but some public, non-editable website.
  3. While the logo alternative text seems good enough, I wonder if we shouldn't offer an easy way to configure the alternative text next to the logo upload.

@Sereza7 Sereza7 marked this pull request as draft October 11, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport stable-15.10.x Used for automatic backport to 15.10.x branch. backport stable-16.4.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants