Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Search Block: Add support for setting the border radius #25553

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Sep 23, 2020

Description

This PR initially extended the work done in #25203 which has since been abandoned in favour of this.

Changes Included

  • Add support for setting border radius on search input field and button in the Search block.
  • Single border radius setting that will adjust the radius for both together.
  • Adjusts inner border radii values so they remain visually consistent with outer wrapper when button is positioned inside.
  • Also resolves linter issues and an extraneous " character in the markup.

Notes

  • Only uses simple formula: innerBorderRadius = wrapperBorderRadius - wrapperPadding
  • This isn't an exact fix as it doesn't take into account border width and padding variations that could be introduced via themes or maybe Global Styles.

Screencast

How has this been tested?

Manual Testing.

Testing Instructions

  1. Apply this PR
  2. Add multiple search blocks to a post
  3. Configure each search block's border radius and button positions differently. Including:
    • Button outside: no border radius
    • Button outside: border radius
    • Button inside: no border radius
    • Button inside: border radius < 4px (less than the wrapper's padding, should result in 1px inner radius)
    • Button inside: border radius > 4px (greater than wrapper's padding, results in (outer radius - padding))
  4. Inspect editor markup and confirm calculations
  5. Save post and view on frontend
  6. Inspect markup again confirming calculations on frontend

Screenshots

Editor Frontend
Screen Shot 2020-09-23 at 4 23 29 pm Screen Shot 2020-09-23 at 4 23 35 pm

Types of changes

Enhancement.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

apeatling and others added 3 commits September 24, 2020 10:30
When the button is placed inside and the inner elements have a matching border radius to the outer wrapper we get "thick" corners where it's a little out of sync. This smooths that out although is not a perfect solution.
@glendaviesnz
Copy link
Contributor

This tests well for me apart from the console warning mentioned.

@aaronrobertshaw
Copy link
Contributor Author

Thanks for the review @glendaviesnz

I've made a few updates to address the issues highlighted if you wouldn't mind taking another look.

@glendaviesnz
Copy link
Contributor

👍 LGTM

Probably a different PR, but might be good to get the Search button to match the front end styles in the editor like the standard button block does.

@aaronrobertshaw
Copy link
Contributor Author

Agreed. Some themes (e.g. TwentyTwenty) do not set the background for the wrapper when the button is positioned on the inside as well. Add this to not currently being able to take padding and border width into account for the border radius calculations and there is still a lot we can iterate on. Once Global Styles are in place we should be able to address a lot of this.

@aaronrobertshaw aaronrobertshaw changed the title Search Block: Reduce inner border radius when button inside Search Block: Add support for setting the border radius Sep 24, 2020
@youknowriad
Copy link
Contributor

I wonder if this should be added adhoc like the current PR or more as "Block support flag" to be able to auto enable it on several blocks (like we do now for lineHeight, colors, spacing support...). Do you think it's possible too try that approach separately in order to get a better understanding of the best approach here?

@aaronrobertshaw
Copy link
Contributor Author

Good idea. I'm more than happy to try out the suggested approach in a new PR.

@aaronrobertshaw
Copy link
Contributor Author

@youknowriad I've had a go at adding border radius block support in #25791

The PR has gotten a little large as I was updating the Search block to leverage the block support for border radius at the same time. If it would make it easier for people to review, I can split that PR into two.

I think your suggested approach works out for the best, making border radius support available to any block that wants it. It didn't reduce the changes to the Search block greatly due to the additional work there to adjust inner elements' border radii so they were consistent with the outer border radius visually. All in all, the block support approach gets my vote.

Any thoughts, suggestions or feedback is more than welcome 🙂

cc @apeatling @glendaviesnz

@glendaviesnz
Copy link
Contributor

I think #25791 is a better way forward so would suggest we go with that approach if others agree.

@apeatling
Copy link
Contributor

+1 on #25791

@aaronrobertshaw
Copy link
Contributor Author

Great, I'll close this in favour of #25791 then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants