-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Improve color contrast of the default block inserter shortcuts. #12928
Improve color contrast of the default block inserter shortcuts. #12928
Conversation
Thanks for the PR and ping. Given that we mean to retire those, is it actually worth making this change? The question here is not one of whether one is better than the other, rather: which WordPress release will this ship with? If it's 5.1 then I would be disappointed if we couldn't fix the problems with this interface at the root, in #10519. So expanding this for a wider review from the group. |
Yup, totally agree it depends on when the refactoring will happen. If it's going to happen soon, in the next release, I'd agree to just close this PR. Otherwise, I'd like to temporarily fix the contrast issue. Thank you. |
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.
Personally I too would like to see these retired. Leaving them with bad contrast on focus isn't a friendly message to users who have trouble with low contrast, but realistically I don't want them to be visible at all so I'd say they're getting a better deal 😉
I think making them more obvious here isn't a good stopgap because it's a worse visual UX for users who can already make out the icons. I know that's not a great answer but I think it's better to leave them as-is until we remove. If someone made a PR that just removed them though I'd approve it ASAP 😄
Looks like #10519 / #11329 won't make it for 5.1. Can we improve this color contrast in the meantime? /Cc @jasmussen @tofumatt |
1b896cf
to
9ea2adf
Compare
Yes, I think we should. Because current master isn't great. Here's 5.0.3: Notice how the contrast is sufficient in the initial placeholder, but it is not sufficient in an "empty paragraph placeholder". Here's this branch: So outside of the contrast changes for the empty paragraph, at least this branch also makes it consistent. However, the fade effect is missing from the left plus on a new post, and it's also missing from the shortcuts on the right. That's not new to this branch specifically, but seems worth fixing while we're at it: In other words: can we make it so all the shortcut icons in placeholders fade in, whether in the initial appender, or the empty placeholders? If yes, then I think we can ship this as a good interim improvement. |
@tofumatt see my previous comment:
Those are now rendered only when hovered. So when rendered, they're already hovered and there's no state transition the CSS transition can understand. |
I'm not sure that's true. I believe that But it's not the end of the world to ship this without those improvements, just seems like it might be worth it. Though it's worth noting that the fade effect is missing on the empty paragraph placeholder in this branch, but it is present in master. |
|
Check better 🙂 This PR touches only colors. |
🤦♂️ I blame mondays. Anyway, no objections to getting this in, as it adds a little consistency. Would be nice if we could restore the fading. |
CC: @youknowriad |
Yep, I'm looking into it. |
Even the most simple things... 🙂 turns out the effects can only work consistently if the elements have the same rendering logic. Basically when an element is always rendered in the DOM, it can get both fade-in and fade-out effects. Instead, when an element is added to the DOM, it can get the fade-in effect via the keyframe animation but when removed it's just removed... there's no time for transitions or animations. To have a fed-out effect, either the element should always be rendered or we should detect In the last commit: editor-default-block-appender:
side-inserter and empty-block-inserter:
Going to check if the side-inserter "plus" can be changed to always be rendered in the DOM. |
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 didn't think it was worth investing in improving these** as my impression was that they're be removed. I don't think they're a good UX regardless of their accessibility.
I'd rather they just be removed.
That said, seems the inserter UX is changing in #10519 so I'm fine with this as a stop-gap.
* Improve color contrast of the default block inserter shortcuts. * Restore fade in effect.
* Improve color contrast of the default block inserter shortcuts. * Restore fade in effect.
As mentioned in #10519 (comment) the default appender shortcuts icons and the default block shortcuts icons are styled differently.
In the default appender, the shortcuts do have a sufficient color contrast:
However, when the default appender is clicked, it becomes the "default block" and the icons don't have a sufficient color contrast:
This small PR seeks to improve the contrast and simplify the CSS:
Worth noting there are also some CSS rules related to the opacity transition that don't work as expected after #12510. I guess this should be addressed separately together with the refactoring proposed in #10519.