SearchControl: remove margin overrides and add new opt-in prop #46081
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
brookewp
added
[Type] Enhancement
|
|
32 tasks
|
Size Change: +66 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
ciampo
approved these changes
Nov 29, 2022
There was a problem hiding this comment.
Code changes LGTM and test well as per the instructions 🚀
We can merge one that (tiny) comment has been addressed :)
Thank you @brookewp for working on this — I really appreciate the clarity of your instructions, it made the reviewing process much easier and it will give anyone stumbling upon your PR a very clear context of the changes!
For TemplatePartSelectionModal
! Note: I think it looks better with a smaller margin to match the top, but in this PR I've added padding to make it look the same as before. I'd be happy to remove it, though, if anyone agrees.
I don't personally have a strong option either way, so I'd say that your proposed solution looks good!
Unrelated to the changes in this PR, but — while testing for this change I noticed that the search bar's parent element doesn't span the whole width of the modal, which means that if scroll down the content of the modal and then hover over a pattern, the focus ring shows to the side of the search bar. Definitely something minor and not for this PR, but I though I'd flag it for a potential follow-up.
| padding: 0 $grid-unit-20; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Spacing nit: I don't think we need this empty line?
There was a problem hiding this comment.
Agreed! That was unintentional but is fixed now. :) Thank you!
brookewp
force-pushed
the
update/searchcontrol-margin
branch
from
e81d04e
to
020f6dd
Compare
mpkelly
pushed a commit
to mpkelly/gutenberg
that referenced
this pull request
Dec 7, 2022
…ress#46081) * SearchControl: remove margin overrides and add new opt-in prop * remove extra line and padding from TemplatePartSelectionModal to match approved proposed solution
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.
What?
Added new opt-in prop
__nextHasNoMarginBottomfor useages ofSearchControlin the Gutenberg codebase and removed margin overrides.Why?
Part of this project: #38730
The tl;dr is
BaseControlhas amargin-bottomwhich makes it difficult to reuse and results in inconsistent use.How?
By removing margin overrides in the CSS and adding the prop
__nextHasNoMarginBottom.Testing Instructions
Testing steps done in the block editor:
For
InserterMenuFor
QuickInserter! Note: Currently, the border around the quick inserter is broken due to the added margin (see gaps in border on left and right side of search). The change here will fix it.
PatternsExplorerSearch
For
BlockManagerTesting steps done in the site editor:
For
TemplatePartSelectionModal! Note: I think it looks better with a smaller margin to match the top, but in this PR I've added padding to make it look the same as before. I'd be happy to remove it, though, if anyone agrees.
For
SuggestionList! Note:For this, you need to have at least 10 or more categories / authors / tags etc or you could edit the following line 127 to have a length greater than 0:
if ( ! showSearchControl && suggestions?.length > 9 ) {For
ScreenBlockList