Fix template part border states. #28241
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Addison-Stavlo
requested review from
mattwiebe,
vindl,
Copons,
david-szabo97,
jeyip and
noahtallen
|
Size Change: +456 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
jameskoster
requested changes
Jan 15, 2021
Addison-Stavlo
force-pushed
the
fix/template-part-border-states
branch
from
4761187
to
f6ad445
Compare
Oh great catch! Now I am remembering why we put those extra rules in previously for exactly that reason. That should be good now.
I pushed over this commit. I think we need to be careful about border colors, the old gray border that was developed when we were on a white background wasnt visible enough when we moved to the green of tt1 blocks. Gray borders aren't consistently going to have the right level of appearance across different background colors, while we can always use the standard wordpress theme and dark theme focus colors and be consistent. |
Expanding on this. previously higher shades of gray were seen as 'too dark' for white backgrounds, in which the desired shade against white was determined to be gray-300. On the default tt1 seafoam green background however, gray-300 is almost invisible. With other shades we don't really have a happy medium of gray that works well for all background colors, especially considering the user can change their background to any of those colors. Im not sure what the BEST solution is, but for now I think using theme/dark-theme focus colors is the most consistent way to go about it - they will always be visible in the same circumstances all of our other border states are visible. Any thoughts on this @jameskoster ? |
|
I don't disagree with anything you've said :) But I do think we should address any color issues from a state of consistency with how blocks behave currently, which is to use a
If there's a problem with this, let's address it all in one go? |
Oh interesting! Do you know if there is another color used for "dark themes"? Or is it always gray-900 right now? |
If im reading through the CSS correctly it looks like no. I see dark theme definitions for some focus states, but 'selected not focused' seems to always be $gray-900. In that case do you think both of these border states for the template part block should be
|
Thats the state of this PR now with ef60c11. Let me know how that feels to you please. 😁 |
15 tasks
jameskoster
approved these changes
Jan 18, 2021
There was a problem hiding this comment.
Thats the state of this PR now with ef60c11. Let me know how that feels to you please. 😁
I think this is good for now – we're in alignment with regular block behaviour, so any issues that exist there can be addressed separately in one go.
This sounds good! Id also note that maybe template parts may make sense to not always follow 'regular block behavior'. After all they are substantially different from regular blocks, they are wrapper blocks that control a completely separate entity in the database and some visuals should be different to help convey that. I think the current state of child-selected border + spotlight mode handles that decently though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Similar to the CSS for the entity spotlight, Template Part border state styles were stripped in #27271. The noted PR was to test showing block borders on hover, but removed a very important non-conflicting border state that is present for entity editing awareness - the has-child-selected state. This state has since been asked to be re-implemented via PR #27780.
This PR re-introduces the has-child-selected border state for template parts so that when a child block in a template part is selected a small outline for the template part will be visible. This state is important for understanding what is/is not part of the template part currently being edited and works together with the entity spotlight system (being fixed here #28239) to visually highlight this for the user.
How has this been tested?
Screenshots
Types of changes
Checklist: