-
Notifications
You must be signed in to change notification settings - Fork 805
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
Jetpack button: fix width and alignment settings #41139
base: trunk
Are you sure you want to change the base?
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
@@ -90,6 +84,5 @@ export function ButtonEdit( props ) { | |||
} | |||
|
|||
export default compose( | |||
withColors( { backgroundColor: 'background-color' }, { textColor: 'color' } ), | |||
applyFallbackStyles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applyFallbackStyles
uses the withFallbackStyles
high order component from @wordpress/components
, which renders the unwanted extra div (at this line). The code suggests we could get rid of that extra div by specifying a node
prop, but it's unclear to me what node we could pass at that stage.
In the following discussion, not using applyFallbackStyles
was hinted by several persons: p1736773641027939/1736360651.812989-slack-C052XEUUBL4. I do think it's the way to go as well. As far as I can tell, what applyFallbackStyles
does is make the default background and text colors available to ButtonEdit
. Yet eventually, those two colors are passed to the ContrastChecker
component, which doesn't render a warning when the contrast ratio is too low to be deemed accessible.
@@ -52,6 +52,10 @@ | |||
flex: 0 0 100%; | |||
margin: 0; | |||
|
|||
&.wp-block-jetpack-button { | |||
flex-basis: auto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overwrite the 100%
value. We want to be able to set the width through the button styling options in the editor.
@@ -226,7 +220,7 @@ function get_button_wrapper_styles( $attributes ) { | |||
$has_width = array_key_exists( 'width', $attributes ); | |||
|
|||
if ( $has_width ) { | |||
$styles[] = 'max-width: 100%'; | |||
$styles[] = sprintf( 'width: %s;', $attributes['width'] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply the width to the root of the jetpack/button
, instead of its __link
element that's now 100% wide.
|
||
usePassthroughAttributes( { attributes, clientId, setAttributes } ); | ||
useWidth( { attributes, disableEffects: isWidthSetOnParentBlock, setAttributes } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling this hook results in resetting the button width when it is aligned left or right. I don't get the rationale behind that decision. In the current UI, from a user standpoint, this doesn't seem like something we'd want.
@@ -39,6 +34,7 @@ export function ButtonEdit( props ) { | |||
|
|||
const blockProps = useBlockProps( { | |||
className: clsx( 'wp-block-button', className ), | |||
style: { width }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set the width
to the root instead of the __link
element.
Setting the width and alignment of a
jetpack/button
block doesn't work consistently, depending on how it is integrated with its parent block and whether we're in the editor or site frontend.For instance, in a Form block, setting the button width to 50% in the editor shrinks it instead of making it half the container's width.
In a Mailchimp block, trying to align the button (to the right in the capture below) doesn't work.
The following blocks use the
jetpack/button
block and show one or both faulty behaviors we've highlighted: Form, Premium Content, Mailchimp, Calendly, Eventbrite, Recipe, and Recurring Payments. The AI Assistant might as well, though I haven't been able to experience it.Proposed changes:
To address that, this PR does two things in the
jetpack/button
block:div
from the markup generated by the blockThis
div
prevents the flexbox layout applied to.block-editor-block-list__layout
from working as intended..wp-block-button
instead of.wp-block-button__link
.Indeed, this Gutenberg PR forces
.wp-block-button__link
to have a width of 100%, breaking the width settings behavior.Furthermore, the PR also updates the blocks consuming
jetpack/button
so the width and alignment settings work as expected. Here are some caveats:jetpack/button
in the Recipe block doesn't work as one would expect, given the width is relative to the grid items of the block. Solving this should be part of a broader discussion.jetpack/button
of a Form is confusing when it's not in its row. It deserves a separate discussion too.Other information:
Jetpack product discussion
n/a
Does this pull request change what data or activity we track or use?
No.
Testing instructions: