Modal: Add small top padding to the content so that avoid cutting off the visible outline when hovering items #51829
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
|
Size Change: -23 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
t-hamano
added
[Type] Bug
t-hamano
added
the
Backport to WP 6.7 Beta/RC
|
Since Gutenberg 16.1 has already been released, I have added the backport label just in case. |
|
Relevant: #51806. CC: @jameskoster |
|
Is there any way we can fix this in the modal component itself? I.e. right now there's a little bottom padding baked into the modal header: 
But there's no top padding baked into the content area: 
Could we split that padding in two, have half be bottom padding on the header, and the other half be top padding on the content? |
|
If we split the padding then the stroke which appears beneath the header on scroll might look a bit close to the title text. Adding 4px padding-top to |
|
This problem will always occur when a focusable element, such as a button, textbox, etc., is placed at the beginning of the modal content. Therefore, it might be a good idea to add slight padding to the top of the modal content. Is there any problem with the approach of adding a slight padding at the top of the
|
|
I understand the problem, but we shouldn't be using internal components classnames (like Couldn't we add small |
|
Thanks for the advice, @ciampo.
I would like to try the latter method so that we don't have to add |
t-hamano
force-pushed
the
fix/add-template-cutoff
branch
from
dcc5ec1 to
7e91b1e
Compare
t-hamano
changed the title
|
Flaky tests detected in c91ada2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5385980149
|
|
I have changed this PR as follows:
As a result, the target to be fixed by this PR has changed. Please see details in the description of the updated PR. |
ciampo
approved these changes
Jun 24, 2023
| @@ -130,7 +130,7 @@ | |||
| .components-modal__content { | |||
| flex: 1; | |||
| margin-top: $header-height + $grid-unit-15; | |||
| padding: 0 $grid-unit-40 $grid-unit-40; | |||
| padding: $border-width-focus-fallback $grid-unit-40 $grid-unit-40; | |||
There was a problem hiding this comment.
It may be worth to add a short inline comment explaining the reason for the top padding
|
I would be tempted to use 4px, so that the overall dimensions continue to align with the 4px baseline grid. 4px would also account for more elaborate focus styles, such as pattern previews, which have an effective 3px outline (1.5px white + 1.5px admin theme color): 
|
|
This seems to solve the issue in a generic way. Top padding is the same: 
Content padding is now 2px at the top: 
However if I don't misread this, that means the spacing between the header title and the body content is now 2px taller, which puts the metrics off a little bit. So to simplify the whole thing, I would:
So for the header instead of |
The Bottom padding is 12px
Bottom padding is 8px
The approach of changing the top padding of the modal content from 2px to 4px seems to have less visual impact. What do you think? |
|
It's a bit hard to discern the difference between those two screenshots, which to me suggests it's okay for that text to drop a bit. The default metrics should still ideally be somewhat close to this grid: Hope that helps! |
|
I have made the following changes:
This looks fine to me. Below are some screenshots. Preferences
Keyboard shotycuts (Scrollable)
Rename Navigation
Choose Pattern Modal displayed when creating a custom template
Choose Pattern Modal displayed when creating a new post
Choose Pattern Modal displayed when creating a new post (mobile)
Rename Navigation (mobile)
ConfirmDialog (no header)
|
jasmussen
approved these changes
Jun 27, 2023
tellthemachines
pushed a commit
that referenced
this pull request
Jun 28, 2023
… the visible outline when hovering items (#51829) * Site Editor: Fix focus cutoff in add template modal * Add padding-top to the modal content * Remove unnecessary padding-top * Remove unnecessary padding-top * Update changelog * Revert top padding from block patterns list * Revert top padding from block patterns list * Remove unnecessary changes * Remove unnecessary changes * Add CSS inline comment * Change padding metrics
|
I just cherry-picked this PR to the add/pre-beta1-bugfixes branch to get it included in the next release: 4f3599d |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
tellthemachines
added a commit
that referenced
this pull request
Jun 28, 2023
* Restore sidebar in focus mode on Pattern click through in Browse Mode Library (#51897) Brings back #51492 * [Command center]: Add preferences and keyboard shortcuts commands (#51862) * [Command center]: Add preferences and keyboard shortcuts commands * update labels * [Site Editor]: Fix `library` command path (#51837) * Updating social link attributes (#51997) * Try: Update template titles (#51428) * Update template titles * Fix typo Co-authored-by: Alex Stine <[email protected]> * Revert Index rename * "front page" -> "homepage" * Update 404 Page make more sense given the template appears in the Pages panel too. * Front Page * home title + description * Revert Home name change, and move compat files * separate code for wp versions * update tests --------- Co-authored-by: Alex Stine <[email protected]> Co-authored-by: ntsekouras <[email protected]> * Update text color (#51965) * Modal: Add small top padding to the content so that avoid cutting off the visible outline when hovering items (#51829) * Site Editor: Fix focus cutoff in add template modal * Add padding-top to the modal content * Remove unnecessary padding-top * Remove unnecessary padding-top * Update changelog * Revert top padding from block patterns list * Revert top padding from block patterns list * Remove unnecessary changes * Remove unnecessary changes * Add CSS inline comment * Change padding metrics * Rest API: rename navigation fallback classes from WP_ to Gutenberg_ (#51959) * The `WP_REST_Navigation_Fallback_Controller` class has been committed to core and therefore results in a naming conflict and unit test failures. Ideally `WP_REST_Navigation_Fallback_Controller` should have been named `WP_REST_Navigation_Fallback_Controller_Gutenberg` and extended `WP_REST_Navigation_Fallback_Controller`. But we can conditionally load the file instead. * Renamed WP_Classic_To_Block_Menu_Converter to Gutenberg_Classic_To_Block_Menu_Converter Load WP_REST_Navigation_Fallback_Controller dependencies in load.php * Renamed all 6.3 classes to have the Gutenberg_ prefix. This should avoid compat errors and hopefully some confusion later. * Also rename test files for completeness * Updated deprecation notices to refer to Gutenberg classes * Fix phpunit failures (#51950) * Fix phpunit failures * Add comment * Update comment with actual reason this fix works --------- Co-authored-by: Dave Smith <[email protected]> Co-authored-by: Nik Tsekouras <[email protected]> Co-authored-by: Ramon <[email protected]> Co-authored-by: James Koster <[email protected]> Co-authored-by: Alex Stine <[email protected]> Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Robert Anderson <[email protected]>
|
I think this caused a little visual regression on Blocks screen of the Edit Post Preferences Modal: 
Gap above the sticky elements. |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
… the visible outline when hovering items (WordPress#51829) * Site Editor: Fix focus cutoff in add template modal * Add padding-top to the modal content * Remove unnecessary padding-top * Remove unnecessary padding-top * Update changelog * Revert top padding from block patterns list * Revert top padding from block patterns list * Remove unnecessary changes * Remove unnecessary changes * Add CSS inline comment * Change padding metrics
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.
Update:
This PR originally tried to fix the cutoff when focusing on an item in the "Add template" modal. However, with #51806, the text now appears above the item and I have confirmed that this problem no longer occours.
However, the same problem occurs when there is a focusable element at the beginning of the modal content. Therefore, I decided to add 2px top padding to the content of the modal component itself.
What?
This PR adds 2px top padding to the content of the
Modalcomponent. This prevents the problem of the outline being cut off in advance when a focusable element is inserted into the modal content.How?
Use the
$border-width-focus-fallbackvariable to add 2px padding to the top of the modal content.Testing Instructions
Rename Navigation Modal
Choose Pattern Modal
I have found that adding top padding to the Modal component itself fixes the outline cut-off in the following two existing modals:
These modals already have 2px top padding to prevent outline cutting on focus. However, when focused, the outlines are extended by an additional 2px due to the offset, resulting in the outlines being cut off. By adding top padding to the modal component itself, it now has a total of 4px top padding, which also fixes the outline cutoff on focus.