XWIKI-22165: Home page icons do not have a text alternative #3123
Conversation
* Updated the english homepage with screen reader only bits of text.
Sereza7
added
the
backport stable-15.10.x
| @@ -45,11 +45,13 @@ XWiki can be used as a knowledge base (support, documentation, sales, etc.), for | |||
|
|
|||
| To make the most out of your wiki, log-in and: | |||
|
|
|||
| Use the {{displayIcon name="pencil"/}} button above to //edit// this page and start customizing your wiki to your needs. | |||
| Use the {{displayIcon name="pencil"/}}{{html}}<span class="sr-only">Edit</span>{{/html}} button above to //edit// this page and start customizing your wiki to your needs. | |||
There was a problem hiding this comment.
Wouldn't that be better to actually improve the displayIcon macro to be able to insert the alternative text?
There was a problem hiding this comment.
To be honest I'm not a big fan of inserting an {{html}} macro there: it makes the page content more complex.
There was a problem hiding this comment.
There is no need to use HTML, using (% class="sr-only" %)Edit(% %) should give the exactly same HTML but has the disadvantage that it isn't editable in the WYSIWYG editor. I agree with @surli that it might be better to integrate this as a macro parameter to make it easier to discover.
There was a problem hiding this comment.
I agree with Simon, it's very important that the home page be as simple as possible to edit/understand as it's made to be edited and modified. It also acts as example. So -1 from me too to add the html macro.
There was a problem hiding this comment.
Addressed in f1b8e50 👍
* Added a macro parameter for the text alternative. * Added a test to cover this parameter * Updated the content of the home page to use this new parameter
* Renamed the test file to make room for the one that was added in the meantime
|
Fixed the merge conflict. |
...xwiki-platform-icon-macro/src/main/java/org/xwiki/icon/macro/DisplayIconMacroParameters.java
Outdated
Show resolved
Hide resolved
...xwiki-platform-icon-macro/src/main/java/org/xwiki/icon/macro/DisplayIconMacroParameters.java
Outdated
Show resolved
Hide resolved
* Updated the version comments
| public void setTextAlternative(String textAlternative) | ||
| { | ||
| this.textAlternative = textAlternative; | ||
| } |
There was a problem hiding this comment.
@Sereza7 have you tried to build this module with the quality profile? It's an API it seems, and you're missing unstable annotations I think. And I'm surprised revapi is not complaining at all.
There was a problem hiding this comment.
Adding methods to a class isn't breaking anything, so I wouldn't expect any errors.
There was a problem hiding this comment.
Right, not an interface. Still missing the unstable annotations then.
There was a problem hiding this comment.
With the quality profile, I hit the ClassFanOutComplexity on DisplayIconMacro, working on fixing it...
There was a problem hiding this comment.
Added the Unstable annotation in 7d12a73 👍
Also reduced the fanout complexity by choosing to use a paragraph block instead of a FormatBlock for the text alternative. Now passes mvn clean install -f xwiki-platform-core/xwiki-platform-icon/xwiki-platform-icon-macro -Pquality successfully.
There was a problem hiding this comment.
I'm sorry but using a paragraph block for the text alternative is not okay, you cannot nest a paragraph inside another paragraph and the icon macro needs to be usable in a paragraph.
There was a problem hiding this comment.
Reverted this change of block nature in 51edb3b .
Instead of this, I set some methods in their own class in order to reduce the ClassFanOutComplexity of DisplayIconMacro.
The PR with these new changes pass quality tests :)
mvn clean install -f xwiki-platform-core/xwiki-platform-icon/xwiki-platform-icon-macro -Pquality
There was a problem hiding this comment.
I went with a solution without components since those methods are quite specific and I don't think they'd need to be reused in another context.
* Added the unstable annotation on the new class methods * Reduced class fanout on DisplayIconMacro.java and updated the test to take into account this slight change in solution.
Sereza7
added
backport stable-16.4.x
and removed
backport stable-15.10.x
* Reduced ClassFanout complexity by setting some methods in annex module-protected classes. * Updated the version numbers. * Reverted latest changes to make paragraphs instead of format
|
I proposed a new solution to pass the quality gates, see #3123 (comment) This pull request is now ready for reviews. |
Jira URL
https://jira.xwiki.org/browse/XWIKI-22165
Changes
Description
Clarifications
Screenshots & Video
after applying the changes proposed in this PR:
Executed Tests
None except manual (see screenshot above).
Page content only, a text search of the string next to the changes did only return the current document. AFAIK it means there's no test validating this specific content.
Expected merging strategy