Fix transparent verification checkmark in dark mode #30235
Conversation
github-actions
bot
added
the
Z-Community-PR
res/css/views/rooms/_E2EIcon.pcss
Outdated
| @@ -59,3 +59,8 @@ Please see LICENSE files in the repository root for full details. | |||
| mask-image: url("$(res)/img/e2e/verified.svg"); | |||
| background-color: $e2e-verified-color; | |||
| } | |||
|
|
|||
| .mx_E2EIcon_verified .mx_E2EIcon_normal::after { | |||
There was a problem hiding this comment.
Why is one of these the normal selector and one an after?
There was a problem hiding this comment.
The <div> of the normal icon is a child of the <div> of the verified icon - not its ::after pseudo class. Yet the icons get rendered by the ::after so we need this selector to corretly apply color and resizing.
| content = <div className={classes} style={style}><div className="mx_E2EIcon_normal" /></div>; | ||
| } else { | ||
| content = <div className={classes} style={style} />; | ||
| } |
There was a problem hiding this comment.
Not sure I understand this change: aren't the classes supposed to correspond to the state of the icon, so normal and verified are separate states? Why is this adding normal when it's verified? Also, why the nested div?
There was a problem hiding this comment.
I just used the "normal" Icon, to put it behind the verified icon, to use it as a white background. I didn't really know how else to do it, except to use a white rectangle or something. But in any case I needed some kind of additional div to use a a background, because otherwise, the whole box around the verified shield would be white, and that looked bad (in my opinion).
And the "normal" icon already had the right form factor.
But I can change the Icon to a new div with a smaller rectangle. I just thought it would be better to reuse existing icons, but as said, I can easily change that.
There was a problem hiding this comment.
Aha! I hadn't realised that's what this was doing. I think this could definitely do with a comment! I think it will also need the same with the warning state of the icon, as that has a '!' cutout in the shield.
There was a problem hiding this comment.
Then I will add it to that as well
| <div | ||
| class="mx_E2EIcon_normal" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
The nested div is for the background, to not have the white background being bigger than the verified icon.
Hi again, What I am still unsure about are the failing testcases for the playwright tests. I have looked into them, and I am genuinely confused why they are failing, because it seems like it's testcases that have nothing to do with what I changed? |
Also adapted the testcases to the new background.
|
We have flaky tests at the moment so the failures probably aren't related: I've retried and it seems fine now. Also, please don't force-push PRs, it means we can't see what code we've already reviewed. |
|
Thanks for accepting the PR |
fixes #28285
What did I change?
I added a white version of the verification shield as a background to the verification symbol (checkmark) when authenticating a new web-device with another device.
Before:
After:
Why did I change it?
The checkmark in the verification icon was transparent, which lead to a black checkmark in darkmode, while the corresponding checkmark in the other device was white. (See issue description.)
Now it is also shown as a white checkmark.
Testing: