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

[#113] - Radius not an option #117

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

thatisrich
Copy link
Contributor

@thatisrich thatisrich commented Oct 7, 2024

Description

#113 - This PR aims to introduce a border radius option alongside the current border option.

Using the experimental BorderRadiusControl component, a border radius can be applied using a singular value or 'unlinked' values for each corner of the element.

This PR also addresses a slight mis-match in label font-weight by changing the value assigned to .themer--styles__item__label from 600 to 500.

Change Log

  • src/editor/components/StylesBorder.js - Including radius component in the layout
  • src/editor/components/StylesBorderRadius.js - The new component to handle the radius values with the BorderRadiusControl component
  • src/editor/styles/components/styles.scss - Updates the custom label font weight

Steps to test

  • Navigate to the Styles Editor page
  • Select an element or block, such as Button
  • Scroll down in the styles panel to the bottom where Border and radius is in view
  • Adjust the Radius Linked value using the input box
  • When the Visual view is selected, notice the border radius will change accordingly
  • When the Code view is selected, notice the value of the radius property will change accordingly
  • Adjust the Radius Linked value using the range slider
  • Notice the changes are reflected in both views
  • Click on the Unlink button to reveal 4 input boxes, one for each corner of the element
  • Update the value in each box
  • Notice the changes are reflected in both the Visual and Code views
  • Follow these steps on other blocks and elements

Screenshots/Videos

A demo showing various interactions with the radius setting and the visual preview
A demo showing various interactions with the radius setting and the code preview
An updated demo showing the border radius setting in visual preview
An updated demo showing the border radius setting in code preview

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@thatisrich thatisrich marked this pull request as ready for review October 7, 2024 17:39
@thatisrich thatisrich requested review from g-elwell and a team as code owners October 7, 2024 17:39
@thatisrich
Copy link
Contributor Author

On further testing I noticed a bug in which the value would be stripped when toggling between Linked and Unlinked values. This has now been addressed and works as expected and fallbacks are in place.

A demo showing how the toggles work when stripping certain values can be seen here

@g-elwell g-elwell changed the base branch from feature/block-preview to develop November 25, 2024 11:58
Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @thatisrich - thanks for working on this! It works well during my testing, however whilst comparing things to Gutenberg I noticed there is a BorderRadiusControl component in there, were you aware of this, do you think we'd be able to update this PR to utilise it?

@thatisrich thatisrich requested a review from g-elwell January 3, 2025 17:21
Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into the WordPress component, this works well and using it means less maintenance for us 🙌

I don't think border radius will be needed as a standalone component, so IMO we should just use the BorderRadiusControl within the StylesBorder component, and add a new onChange callback for it, just to keep things a bit neater, e.g:

const onRadiusChange = ( newValue ) => {
	let config = structuredClone( userConfig );
	config = set( config, `${ selector }.radius`, newValue );
	setUserConfig( config );
};

setUserConfig( config );
};

return (
<>
<span className="themer--styles__item__title">
{ __( 'Border', 'themer' ) }
{ __( 'Border and radius', 'themer' ) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can we keep this as "Border" as radius is just a part of the border styles

@thatisrich
Copy link
Contributor Author

When further testing the BorderRadiusControl component after moving it, I noticed an issue when a deleted radius value did not have a fallback resulting in an unexpected border radius being applied in both Themer and on the front end. The onRadiusChange function has been adapted to account for this and, (thanks to the suggestions!) should streamline the StylesBorder component and maintenance moving forward.

Please see here for an updated screen recording showing the minor UI tweaks.

@thatisrich thatisrich requested a review from g-elwell January 6, 2025 10:50
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.

2 participants