XWIKI-21597: Make the rights UI use icon themes #2649
Conversation
* Updated icons in the rights UI
* Removed the logic to update the source for the checkbox image
| this.icons = [ | ||
| "", | ||
| "$escapetool.javascript($services.icon.renderHTML('check'))", | ||
| "$escapetool.javascript($services.icon.renderHTML('cross'))" |
There was a problem hiding this comment.
Would be cleaner to move out the velocity calls, as it's recommended in https://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices#HJavaScriptBestPractices (and what you used in https://github.com/xwiki/xwiki-platform/pull/2648/files#diff-14e05ae329b087de0d0f45b460cdb1b33da4382f3d9c806125daf81cfac152bbR51 for example)
There was a problem hiding this comment.
Addressed in 78ca48e 👍
I tested this change manually and as far as I could see it did not break anything (at least I fixed mistakes until I could notice it working again :) ).
* Removed velocity calls from inside the javascript
|
Fails webstandard tests, |
* Removed the alt - didn't add an alternative because what it used to be is already supported by the default checkbox behaviour * Updated the javascript to fit this change in marking even when checking/unchecking the boxes.
|
With the changes from b2f2a7a, testing Everything good, opening this PR back for reviews 👍 |
| border-color: transparent; | ||
| background: transparent; | ||
| #usersandgroupstable button.rights-edit.yes { | ||
| color: $theme.notificationSuccessColor; |
There was a problem hiding this comment.
Feels a bit weird here to use notification(Success|Error)Color, now maybe that's the best practice in themes?
There was a problem hiding this comment.
That's some of the old colortheme variables. I'm not sure we have a best practice for this. I think I did use those old variables since others are used all throughout the file and I'd rather keep consistent inside one file.
To be honest this whole file looks quite old and breaks current CSS codestyles in multiple places.
I opened a topic on the forum about introducing a new type of label for codestyle, this file could really use a ticket :)
I just checked our HTML and CSS best practices and the entry don't hard code colors in properties - use [ColorTheme variables](https://extensions.xwiki.org/xwiki/bin/view/Extension/Color%20Theme%20Application#HUsingColorThemesvariables) points to a doc that uses the old colortheme variables. We might want to update the link or the doc.
I'll look a bit more into the state of deprecation of this old colortheme, it's not clear to me what it is yet. Using the old colortheme everywhere does not look like something we should recommend...
There was a problem hiding this comment.
I updated the doc to include a link to how to use the new variables.
Jira
https://jira.xwiki.org/browse/XWIKI-21597
PR Changes
View
After the PR
We can see that this PR updates the icons both in the right assignment field and the fields under. UInder, this is just a two state checkbox, which can be handled by the default HTML, so I did not use the icon theme. However in the right assignment fields, I changed the icon for the three states. For the none state, which was previously shown with an empty box, I decided to leave the button visuals empty.