-
-
Notifications
You must be signed in to change notification settings - Fork 518
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-21216: Lack of contrast on hover of lightbox buttons when expanded #2297
base: master
Are you sure you want to change the base?
Conversation
* Replaced hard coded colors with panel values from the color theme.
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.
From a technical point of view I agree. However, making the background white/gray instead of black is quite a change and not typical for this kind of lightbox, so I think we should have a forum proposal for this.
opacity: 1; | ||
} | ||
|
||
.action { | ||
color: @xwiki-page-content-bg; | ||
color: @panel-default-text; | ||
opacity: 0.8; |
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 thought it was a bad idea to use opacity with colors?
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 chose to use an opacity of 0.8 here since we don't have a variation of @text-muted
that would match the color in @panel-default-text
(something like @panel-muted-text
). See this ticket and this topic on the forum for a discussion about the state of panel colours in the colorthemes.
Opacity can reduce the importance of a text on both light and dark theme, which is not true for the LESS color operation functions lighten/darken.
This is not a perfect solution, but I think this is the one with the least drawbacks.
background-color: rgba(0, 0, 0, 0.6); | ||
color: #FFFFFF; | ||
background-color: @panel-bg; | ||
opacity: 0.6; |
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.
Isn't the opacity dangerous in terms of contrast?
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.
It was already there in the previously hard coded color. I think this is too much of a change to remove it and there is a genuine point to opacity here (we want to still see content behind).
I insisted to remove it on some other PRs, because transparency was not necessary but just a mean to change the color, and we had easy alternative with full semantically appropriate colors from the theme.
Started a forum topic about the solution proposed here |
Conclusion of the forum feedback:
|
* Added back hard coded color, but removed the color theme ones so that we don't break contrast on half the themes.
Jira
https://jira.xwiki.org/browse/XWIKI-21216
PR Changes
View
Hereafter are four screenshots of the involved changes. They come in pairs, the first being the state before the changes proposed here, and the second being the state after the changes proposed here. On all of those screenshot, the :hover style has been applied on the leftmost button. The green highlight is created by an extension.
(a) Modal with the Iceberg color theme, before
(b) Modal with the Iceberg color theme, after
(c) Modal with the Darkly color theme, before
(d) Modal with the Darkly color theme, after