-
-
Notifications
You must be signed in to change notification settings - Fork 599
XWIKI-22602: Provide a UI to easily customize the logo alternative text from the colorTheme #4517
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
Conversation
…xt from the colorTheme * Updated the flamingo theme class to add a text alternative * Updated the theme editor * Used the new variable in the template (default is the value used previously)
…xt from the colorTheme * Changed the prettyName to fit with other prettyNames in the ThemeClass
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.
Thank you very much! I think in general this is a great feature, my primary complaint is that it is probably non-obvious for admins where this value is used and how it should be set.
...-theme/xwiki-platform-flamingo-theme-ui/src/main/resources/FlamingoThemesCode/ThemeClass.xml
Outdated
Show resolved
Hide resolved
...-theme/xwiki-platform-flamingo-theme-ui/src/main/resources/FlamingoThemesCode/ThemeSheet.xml
Show resolved
Hide resolved
…xt from the colorTheme * Updated the object number
…xt from the colorTheme * Added the hint on the logo-description field
@@ -745,6 +754,12 @@ require(['jquery', 'xwiki-l10n!xwiki-flamingo-theme-messages', 'bootstrap'], fun | |||
|
|||
#iframe { | |||
height: 400px; | |||
} | |||
|
|||
.xHint { |
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.
Would be nice if we didn't need that. Putting the xform class on any parent element wasn't an option/would break too much?
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.
According to https://www.xwiki.org/xwiki/bin/view/Documentation/DevGuide/FrontendResources/VerticalForms/ , we should put it on the parent form
tag.
I tested quickly in the inspector and it seems like it overrides the custom boldness of field names:

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.
Yeah, probably not good to perform this change now. Maybe just add a comment in the CSS that this should be refactored to properly use the vertical forms standard (currently, it's missing the wrapping definition list).
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.
Looking at this again, I just noticed that this selector is very broad. Please scope this selector to really only affect the color theme editing form.
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.
Since this is a On demand only
style sheet used only in this context, I figured it was okay. Note that extension.css
has a similarly very wide selector for the class:
Lines 443 to 447 in c91ee94
.xHint { | |
color: $theme.textSecondaryColor; | |
font-size: smaller; | |
margin: 0.3em 0; | |
} |
I increased specificity in 1f101d5 👍
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.
These kinds of wide selectors tend to create problems when other UI elements use the same class. Imagine a panel containing a form with a hint. I remember fixing a similar issue with the quick search that was influenced by some other CSS (but I can't find the Jira issue anymore).
@@ -108,6 +108,7 @@ platform.flamingo.themes.defaultvalue=Default | |||
platform.flamingo.themes.lessCode.description=In this field, you can directly write LESS code which will be added into the skin. | |||
platform.flamingo.themes.local=Local | |||
platform.flamingo.themes.global=Global | |||
FlamingoThemesCode.ThemeClass_logo-description.hint=Alternative text to the logo. It should describe the logo's meaning. Default: "Logo of the wiki" |
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 would add a comment above this to mention the other translation key with which the default value needs to be synchronized.
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 added the mention of the translation inside the hint itself, from what you described in your other comment it made more sense.
Added in 913ce37 👍
Seeing the translation with the default value mentioned, I'm wondering something: in a multilingual wiki, shouldn't it be possible to localize the description of the logo? In theory, actually, also the logo itself should be localized (at least some organizations include a slogan in their logo that is translated in other languages)… |
…xt from the colorTheme * Updated the translation to mention the localized value.
I added info in the hint to make it a bit clearer that by using this field, the user is losing on the default localization. In a multilingual wiki, it's still possible to localize the description of the logo, but instead of filling up this field, the user needs to update all the translations. |
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.
Thank you very much for the changes. Overall, this looks good to me, just two last comments.
...heme/xwiki-platform-flamingo-theme-ui/src/main/resources/FlamingoThemesCode/Translations.xml
Outdated
Show resolved
Hide resolved
@@ -745,6 +754,12 @@ require(['jquery', 'xwiki-l10n!xwiki-flamingo-theme-messages', 'bootstrap'], fun | |||
|
|||
#iframe { | |||
height: 400px; | |||
} | |||
|
|||
.xHint { |
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.
Yeah, probably not good to perform this change now. Maybe just add a comment in the CSS that this should be refactored to properly use the vertical forms standard (currently, it's missing the wrapping definition list).
…xt from the colorTheme * Added a comment for further improvements.
Done in 5f1ab3c 👍 |
…xt from the colorTheme * Increased specificity of the added styles
…xt from the colorTheme * Replaced curved quote in the translation with a double quote.
Jira URL
https://jira.xwiki.org/browse/XWIKI-22602
Changes
Description
Clarifications
logo-description
field of the theme class. This means that in the class editor it's far from thelogo
field, but since this class has a custom editor it doesn't matter much. I'd rather do that than increment the numbers of all the fields afterlogo
...Screenshots & Video
Here is a view of the generated DOM with the changes applied and the value

Organization Name
set as the logo description:We can see that the tooltip on hover just displays "Home", but the image has the proper text alternative.
Executed Tests
Built changes successfully with
mvn clean install -f xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-resources -Pquality
andmvn clean install -f xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-theme/xwiki-platform-flamingo-theme-ui -Pquality
Tested the change on my local instance, it worked as expected (see above).
Expected merging strategy