DataViews: set color for primary field/a element when focused
#58814
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
oandregal
commented
Feb 8, 2024
packages/dataviews/src/style.scss
Outdated
| @@ -228,6 +228,10 @@ | |||
| &:hover { | |||
| color: $gray-900; | |||
| } | |||
| &:focus { | |||
| color: var(--wp-admin-theme-color--rgb); | |||
There was a problem hiding this comment.
I'm not convinced this is the right approach/color combination.
There was a problem hiding this comment.
In addition to the fix, I wonder how 3rd parties can provide their own theming. We have a mix of approaches:
- in some places, dataviews uses the
@wordpress/componentvariables and fallback to wp-admin theme. - in some other, dataviews uses the wp-admin theme custom properties directly.
oandregal
added
[Type] Bug
|
Size Change: +49 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
This seems like a step in the right direction to me, but the cut-off shadow looks a bit strange. Could we try a solid ring instead, similar to the focus treatment applied to other elements in Gutenberg? 
That seems achievable using As noted in #58811 should we apply these styles to |
Pushed an update and it's the outline/shadow is no longer cut-off.
The difference is visible for controls ( |
oandregal
force-pushed
the
fix/wp-admin-color-bleed
branch
from
d01629e
to
c061051
Compare
|
After looks good, though can you try an outset box-shadow instead of inset? |
oandregal
force-pushed
the
fix/wp-admin-color-bleed
branch
from
c061051
to
51f5c09
Compare
ok, I've done that. I had to add a little margin on the sides to prevent the outline cut-off. Is that the preferred way? Also tried with outline-offset but couldn't make it work. Feel free to push a fix if you will. |
|
I'll try a fix tomorrow, I don't think we should add margin, or it might get jumpy. I see that the box shadow is already not "inset", so something else may be going on. But I'll see if I can push a fix in my morning. |
|
I pushed a small fix, I think this is acceptable:
However, I'm not entirely sure this should land. I wonder if we can put it in the reset mixin? ( The thing is: we should not override the focus style on a per-section basis. There should be a single focus style for the Here's another place where it bleeds through:
Pending an update to all of WordPress, can we move the fix to the reset, and then apply a local override here if need be? |
oandregal
force-pushed
the
fix/wp-admin-color-bleed
branch
from
d68cbfb
to
3f9c376
Compare
|
@jasmussen I've created a Also, with the latest commit, I still see the outline being cut-off on the inline axis: 
|
|
The mixin sounds good. As far as the cutoff, is there |
|
I think @t-hamano may have worked on adjacent code, perhaps @richtabor as well. |
oandregal
force-pushed
the
fix/wp-admin-color-bleed
branch
from
3f9c376
to
b03904e
Compare
|
ok, removed the overflow hidden and it looks fine, even with long titles: 
|
oandregal
force-pushed
the
fix/wp-admin-color-bleed
branch
from
6314965
to
1a84d9e
Compare
t-hamano
requested changes
Feb 28, 2024
t-hamano
reviewed
Feb 28, 2024
oandregal
force-pushed
the
fix/wp-admin-color-bleed
branch
from
405ce6d
to
6ade827
Compare
t-hamano
approved these changes
Feb 29, 2024
youknowriad
reviewed
Mar 4, 2024
| @@ -367,6 +367,14 @@ | |||
| } | |||
| } | |||
|
|
|||
| @mixin link-reset { | |||
There was a problem hiding this comment.
Why is this called link-reset?
|
Is this fix, something we want in 6.5? |
|
I don't see any reason not to include it. |
youknowriad
added
the
Backport to WP Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
Mar 4, 2024
Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org>
|
I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release: 8334c94 |
youknowriad
added
Backported to WP Core
youknowriad
pushed a commit
that referenced
this pull request
Mar 4, 2024
Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org>
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.
Part of #55083
Fixes #58811