DataViews: Add space for focus outline to primary field #59068
Closed
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
t-hamano
added
[Type] Bug
|
Size Change: +44 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
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. |
|
Flaky tests detected in 8bd3710. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7914579924
|
ntsekouras
reviewed
Feb 15, 2024
| @@ -216,6 +216,10 @@ | |||
| display: block; | |||
| width: 100%; | |||
|
|
|||
| &:has(a) { | |||
There was a problem hiding this comment.
I think the misalignment with the padding won't be great in general.. Maybe we could remove the overflow: hidden; in the main container above? 🤔 --cc @jameskoster
In general it makes me wonder what styles should belong in the DataViews package and which ones should be added by the consumers.. Maybe the link styles should be provided by the consumer if needed, because it could result to unexpected results..
For example in primary field someone could have a paragraph with nested links(a tag). Currently with ship with display: block; styles and would be weird IMO.
There was a problem hiding this comment.
I agree this could lead to inconsistencies in a single data view which isn't ideal. It also breaks the 4px spacing system.
As for whether the styles are built-in, I see that as an implementation detail best left to y'all engineers to decide :) I can see that built-in styles could be unhelpful in some situations. As long as it's possible to apply truncation to page / template / pattern titles in certain layouts as required I'm happy.
Either way, the issue this PR aims to fix will continue to exist. @t-hamano perhaps an alternative approach could be to add 4px horizontal padding to .dataviews-view-grid__primary-field and reduce the gap on dataviews-view-grid__title-actions from 8 to 4px?
There was a problem hiding this comment.
I've got a PR that touches on the same thing but doesn't need to add the padding #58814
There was a problem hiding this comment.
@oandregal I had overlooked that PR 😅 #58814 seems to have a broader discussion going on, so I'm happy to close this PR to prevent cluttering the discussion.
|
I would like to close this PR because I learned that #58814, which aims to fix the same issue, is already in progress. |
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.
Fixes #59053
What?
This PR adds padding to the left and right of the primary field so that the focus outline of the link is not cut off.
Why?
This link has
overflow:hiddenapplied to prevent it from overflowing when the title is long. This also cuts the focus outline.How?
I used the
:haspseudo-class and added 2px padding to the left and right of the wrapper div only if it has a link element. The:haspseudo-class supports major browsers.This 2px value is the width of the new focus outline in the core. There is no variable in Gutenberg that defines the width of this new focus outline. So for now I've hardcoded the value itself.
Testing Instructions for Keyboard
Screenshots or screencast
Focus outline when title is long: