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

Jetpack button: fix width and alignment settings #41139

Merged
merged 8 commits into from
Feb 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions projects/packages/forms/changelog/fix-jetpack-button-styling
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fixed

Fix submit button width and alignment
7 changes: 6 additions & 1 deletion projects/packages/forms/src/blocks/contact-form/editor.scss
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,12 @@

.wp-block {
flex: 0 0 100%;
margin: 0;
margin-top: 0;
margin-bottom: 0;

&.wp-block-jetpack-button {
flex-basis: auto;
Copy link
Contributor Author

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.

}

&.jetpack-field__width-25,
&.jetpack-field__width-50,
Expand Down
25 changes: 22 additions & 3 deletions projects/packages/forms/src/contact-form/css/grunion.css
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@
gap: var(--wp--style--block-gap, 1.5rem);
}

.wp-block-jetpack-contact-form > * {
.wp-block-jetpack-contact-form > *:not(.wp-block-jetpack-button) {
flex: 0 0 100%;
box-sizing: border-box;
}
Expand All @@ -241,8 +241,27 @@
padding-left: 0;
}

.wp-block-jetpack-button.alignright button {
float: right;
.wp-block-jetpack-contact-form .wp-block-jetpack-button {
width: fit-content;
}

.wp-block-jetpack-contact-form .wp-block-jetpack-button.aligncenter {
display: block;

margin-left: auto;
margin-right: auto;
}

.wp-block-jetpack-contact-form .wp-block-jetpack-button.alignleft {
display: block;

margin-right: auto;
}

.wp-block-jetpack-contact-form .wp-block-jetpack-button.alignright {
display: block;

margin-left: auto;
}

.wp-block-jetpack-contact-form .grunion-field-wrap {
Expand Down
4 changes: 4 additions & 0 deletions projects/plugins/jetpack/changelog/fix-jetpack-button-styling
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: other

Jetpack button: fix width and alignment
8 changes: 1 addition & 7 deletions projects/plugins/jetpack/extensions/blocks/button/button.php
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,6 @@ function get_button_styles( $attributes ) {
$has_named_gradient = array_key_exists( 'gradient', $attributes );
$has_custom_gradient = array_key_exists( 'customGradient', $attributes );
$has_border_radius = array_key_exists( 'borderRadius', $attributes );
$has_width = array_key_exists( 'width', $attributes );
$has_font_family = array_key_exists( 'fontFamily', $attributes );
$has_typography_styles = array_key_exists( 'style', $attributes ) && array_key_exists( 'typography', $attributes['style'] );
$has_custom_font_size = $has_typography_styles && array_key_exists( 'fontSize', $attributes['style']['typography'] );
Expand Down Expand Up @@ -206,11 +205,6 @@ function get_button_styles( $attributes ) {
$styles[] = sprintf( 'border-radius: %spx;', $attributes['borderRadius'] );
}

if ( $has_width ) {
$styles[] = sprintf( 'width: %s;', $attributes['width'] );
$styles[] = 'max-width: 100%';
}

return implode( ' ', $styles );
}

Expand All @@ -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'] );
Copy link
Contributor Author

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.

}

return implode( ' ', $styles );
Expand Down
13 changes: 3 additions & 10 deletions projects/plugins/jetpack/extensions/blocks/button/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,16 @@ import {
import { compose } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';
import clsx from 'clsx';
import useWidth from '../../shared/use-width';
import applyFallbackStyles from './apply-fallback-styles';
import { IS_GRADIENT_AVAILABLE } from './constants';
import ButtonControls from './controls';
import usePassthroughAttributes from './use-passthrough-attributes';
import './editor.scss';

export function ButtonEdit( props ) {
const { attributes, backgroundColor, className, clientId, context, setAttributes, textColor } =
props;
const { attributes, backgroundColor, className, clientId, setAttributes, textColor } = props;
const { borderRadius, element, placeholder, text, width, fontSize } = attributes;
const isWidthSetOnParentBlock = 'jetpack/parentBlockWidth' in context;

usePassthroughAttributes( { attributes, clientId, setAttributes } );
useWidth( { attributes, disableEffects: isWidthSetOnParentBlock, setAttributes } );
Copy link
Contributor Author

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.

Copy link
Contributor

@talldan talldan Feb 3, 2025

Choose a reason for hiding this comment

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

I've been testing and I also couldn't see the use case that the hook caters to. I saw this was implemented in #25394 by @mmtr, maybe Miguel remembers.

If there's no apparent current use cases, I wonder if it can be removed completely from the codebase. It's also used by Payment Button, but that has no alignment tool on the toolbar, so perhaps it's not needed.

edit: it just occurred to me, a case where it might makes sense is when the button is 100% width and an alignment is set.


/* eslint-disable react-hooks/rules-of-hooks */
const {
Expand All @@ -39,6 +34,7 @@ export function ButtonEdit( props ) {

const blockProps = useBlockProps( {
className: clsx( 'wp-block-button', className ),
style: { width },
Copy link
Contributor Author

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.

} );

const buttonClasses = clsx( 'wp-block-button__link', {
Expand All @@ -48,7 +44,6 @@ export function ButtonEdit( props ) {
[ textColor.class ]: textColor.class,
[ gradientClass ]: gradientClass,
'no-border-radius': 0 === borderRadius,
'has-custom-width': !! width,
[ `has-${ fontSize }-font-size` ]: !! fontSize,
'has-custom-font-size': !! fontSize,
} );
Expand All @@ -60,7 +55,6 @@ export function ButtonEdit( props ) {
fontSize: attributes.style?.typography?.fontSize,
color: textColor.color,
borderRadius: borderRadius ? borderRadius + 'px' : undefined,
width,
};

return (
Expand Down Expand Up @@ -90,6 +84,5 @@ export function ButtonEdit( props ) {
}

export default compose(
withColors( { backgroundColor: 'background-color' }, { textColor: 'color' } ),
applyFallbackStyles
Copy link
Contributor Author

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.

Copy link
Contributor

@talldan talldan Jan 22, 2025

Choose a reason for hiding this comment

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

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.

It's because the applyFallbackStyles code is wrong. It checks the block's outer element for the background color, but it's actually applied to the inner element (what it calls the textNode).

That said, I think it's pretty trivial to rewrite withFallbackStyles/applyFallbackStyles in a more modern react way that won't result in an extra wrapper, so I agree with removing it, and I'd be happy to work on getting the contrast checker working again in a follow up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a small PR that addresses that - #41294.

I'll keep it as a draft until this or another fix for the styles is merged.

withColors( { backgroundColor: 'background-color' }, { textColor: 'color' } )
)( ButtonEdit );
48 changes: 22 additions & 26 deletions projects/plugins/jetpack/extensions/blocks/button/editor.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
.wp-block[data-type='jetpack/button'] {
display: inline-block;
margin: 0 auto;
@import "../../shared/styles/align";

// This level of CSS specificity is required to overwrite existing styles
.editor-styles-wrapper .block-editor-block-list__block.wp-block-jetpack-button {
@include align-block;
max-width: 100%;
width: fit-content;
margin: 0;

&.is-style-outline > .wp-block-button__link {
background-color: transparent;
Expand All @@ -9,34 +14,25 @@
}
}

.wp-block[data-align="center"] {
.wp-block-jetpack-button {
display: flex;
justify-content: center;
}
// Support align feature for older themes
.wp-block[data-align]:has(> .wp-block-jetpack-button) {
margin-left: 0;
margin-right: 0;
}

.wp-block[data-align="right"] {
.wp-block-jetpack-button {
display: flex;
justify-content: flex-end;
}
.wp-block[data-align] > .wp-block-jetpack-button {
display: block;
}

div[data-type="jetpack/button"]:not([data-align='left']):not([data-align='right']) {
width: 100%;
.wp-block[data-align="center"] > .wp-block-jetpack-button {
margin-left: auto;
margin-right: auto;
}

div[data-type="jetpack/button"][data-align] {
z-index: 1;
width: 100%;

.wp-block > div {
max-width: 100%;
}
.wp-block[data-align="left"] > .wp-block-jetpack-button {
margin-right: auto;
}

.wp-block-jetpack-button,
.wp-block-button__link.has-custom-width {
max-width: 100%;
}
.wp-block[data-align="right"] > .wp-block-jetpack-button {
margin-left: auto;
}
10 changes: 9 additions & 1 deletion projects/plugins/jetpack/extensions/blocks/button/view.scss
Original file line number Diff line number Diff line change
@@ -1,14 +1,22 @@
@import "../../shared/styles/align";

.amp-wp-article .wp-block-jetpack-button {
color: #ffffff;
}

.wp-block-jetpack-button {
@include align-block;
max-width: 100%;
width: fit-content;
margin: 0;

&.is-style-outline > .wp-block-button__link {
background-color: transparent;
border: solid 1px currentColor;
color: currentColor;
}

&:not(.is-style-outline) button {
border: none;
}
}
}
4 changes: 4 additions & 0 deletions projects/plugins/jetpack/extensions/blocks/calendly/view.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
@import "../../shared/styles/align";

.admin-bar .calendly-overlay .calendly-popup-close {
top: 47px;
}
Expand All @@ -16,5 +18,7 @@
}

.wp-block-jetpack-calendly .wp-block-jetpack-button {
@include align-block;

color: #ffffff;
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
@import '../../shared/styles/align';

.wp-block-jetpack-eventbrite {
position: relative;

.wp-block-jetpack-button {
@include align-block;
}

.components-placeholder__learn-more {
margin-top: 1em;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
@import "../../shared/styles/align";
// Hide the link if an iframe got appended to the container.
.eventbrite__direct-link:not(:only-child) {
display: none;
Expand Down Expand Up @@ -65,3 +66,9 @@
height: 40px;
/* @todo More styling could be copied from Eventbrite */
}

.wp-block-jetpack-eventbrite {
.wp-block-jetpack-button {
@include align-block;
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
@import '@automattic/jetpack-base-styles/gutenberg-base-styles';
@import '../../shared/styles/jetpack-variables.scss';
@import "../../shared/styles/align";

.wp-block-jetpack-mailchimp {
&.is-processing {
Expand All @@ -9,6 +10,8 @@
}

.wp-block-jetpack-button, p {
@include align-block;

margin-bottom: 1em;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@
/* Subscribe/Login buttons */

.wp-block-buttons .wp-block[data-type='jetpack/recurring-payments'] {
--jetpack-payment-buttons-gap: 0.5em;

display: inline-block;
margin: 0 0.5em 0 0;
width: auto;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

/** Subscribe button **/
.wp-block-premium-content-logged-out-view .wp-block-jetpack-recurring-payments {
--jetpack-payment-buttons-gap: 0.5em;

display: inline-block;
margin-inline-end: 0.5em;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@
display: block;
}

.wp-block-jetpack-button,
.wp-block[data-type='jetpack/button'] {
width: 100%;
margin-top: 0;
margin-bottom: 0;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,9 @@
.wp-block-jetpack-recurring-payments.aligncenter .wp-block-jetpack-button {
text-align: center;
}

.wp-block-jetpack-recurring-payments {
.wp-block-jetpack-button {
width: 100%;
}
}
20 changes: 20 additions & 0 deletions projects/plugins/jetpack/extensions/shared/styles/align.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
@mixin align-block {
&.aligncenter,
&.alignleft,
&.alignright {
display: block;
}

&.aligncenter {
margin-left: auto;
margin-right: auto;
}

&.alignleft {
margin-right: auto;
}

&.alignright {
margin-left: auto;
}
}
Loading