Search: Update search block to handle per corner border radii #33023
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
aaronrobertshaw
added
[Type] Bug
|
Size Change: +93 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
|
@apeatling Do you think only partially applying border radii to the search block's input and button when the button is placed inside would negatively impact the expanding search block work? By partially applying border radii I mean something like the screenshots below. Note the lack of border radius on the input's right corners and the button's left corners. My thinking is the transition between the non-expanded view to the ones below might get a bit wonky?
|
7 tasks
|
The transition is setting the width of the input field to zero, so it should be okay? The button would remain with the border radii set. |
Ok, thanks. My misunderstanding was that the expanding search input was going to be a separate option to the "button inside" positioning. If the expanding style means the button and input are separate, without a wrapper, then the application of the border radius styles isn't affected. |
There was a problem hiding this comment.
This is testing nicely for me following all the test steps @aaronrobertshaw!
✅ Continues to render existing search blocks with string-based attribute correctly in the editor and on the front end
✅ Updating an existing search block continues to use the string-based approach (since we haven't replaced the control yet)
✅ Blocks using the new object-based attribute are rendered correctly in the editor and on the front end of the site
✅ Switching themes to one that doesn't support the radius control still preserves the rendering of the radius values
I just left a comment about specificity in the naming of the added PHP function, but other than that, this one looks good to go, to me!
There was a problem hiding this comment.
Thanks for fixing up the function name @aaronrobertshaw! I just thought of another case from testing #31585 where corners might have valid null values, so left another couple of comments. It might just need some extra checks before adding styles to neaten up the styles we're outputting?
There was a problem hiding this comment.
Nice refactor @aaronrobertshaw, this is testing well for me now, with no unexpected empty CSS attributes 👍
Just left a comment about potentially escaping the border radius $key since we're concatenating it directly into an attribute. Though, I'm not sure if it's actually an issue with real-world usage, so it could be more theoretical. Apologies if it's too nit-picky!
andrewserong
approved these changes
Jun 30, 2021
Given it was just a timeout error to start with I re-ran the tests. The failing test then passed however another e2e test failed related to the widgets editor. There's a PR for addressing that I believe: #33066. It mentioned a possible order issue hence the intermittent nature of the failures. A final re-run of the tests had them all failing. I'll wait until tomorrow, then rebase/re-run tests as needed. |
The border radii are still applied in a 1:1 fashion to the corners of the input and button when the button is positioned inside. A future iteration might change this so input gets left corners only and button gets right corner values etc.
aaronrobertshaw
force-pushed
the
update/search-per-corner-border-radius
branch
from
b040c8a
to
75350d9
Compare
|
I've made another update to this to ensure a deprecated numerical border radius value displays correctly in the editor. Previously, it was only correct on the frontend. The PR description, testing instructions and block code snippet have been updated. |
|
Thanks for updating this @aaronrobertshaw, I just gave it another test, and it's still working correctly for me, with the addition of now handling the deprecated numeric based border radius 👍 |
7 tasks
12 tasks
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.
Related: #31585 & #33017
Description
Updates the Search block's ad hoc application of border radius block support styles to handle individual border radii for each corner.
The border radii are still applied in a 1:1 fashion to the corners of the input and button when the button is positioned inside. A possible future iteration might be to change this so the input gets left corner values only and button gets right corner values.
How has this been tested?
Manually.
Screenshots
Types of changes
Bug fix.
(Prevents issue with per-corner border radius block support that will occur when UI PR update lands)
Checklist:
*.native.jsfiles for terms that need renaming or removal).