-
-
Notifications
You must be signed in to change notification settings - Fork 542
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-22165: Home page icons do not have a text alternative #3123
base: master
Are you sure you want to change the base?
Conversation
* Updated the english homepage with screen reader only bits of text.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding methods to a class isn't breaking anything, so I wouldn't expect any errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, not an interface. Still missing the unstable annotations then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the quality profile, I hit the ClassFanOutComplexity on DisplayIconMacro, working on fixing it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* 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.
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