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.

Sign up for GitHub

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

Jump to bottom

Subscribe Block: ensure custom button spacing is correct #28057

Merged
merged 1 commit into from
Dec 22, 2022
Merged

Subscribe Block: ensure custom button spacing is correct #28057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/plugins/jetpack/changelog/fix-sprintf-placeholder
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: bugfix

Subscribe Block: ensure custom button spacing is correct when the button is on its own line.
2 changes: 1 addition & 1 deletion projects/plugins/jetpack/extensions/blocks/subscriptions/subscriptions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ function get_element_styles_from_attributes( $attributes ) {

// Account for custom margins on inline forms.
$submit_button_styles .= true === get_attribute( $attributes, 'buttonOnNewLine' )
? sprintf( 'width: calc(100% - %spx;', get_attribute( $attributes, 'spacing', DEFAULT_SPACING_VALUE ) )
? sprintf( 'width: calc(100%% - %dpx);', get_attribute( $attributes, 'spacing', DEFAULT_SPACING_VALUE ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. Should we actually have another case for buttons with 100% width on their own? By deducting the spacing from the width the subscribe button isn't quite full width which looks a little odd on my test site (on the front-end):

Screen Shot 2022-12-22 at 2 49 53 pm

Something like this:

		if (  get_attribute( $attributes, 'buttonWidth' ) === '100%' ) {
			$submit_button_styles .= 'width: 100%;';
		}
		else {
			// Account for custom margins on inline forms.
			$submit_button_styles .= true === get_attribute( $attributes, 'buttonOnNewLine' )
				? sprintf( 'width: calc(100%% - %dpx);', get_attribute( $attributes, 'spacing', DEFAULT_SPACING_VALUE ) )
				: 'width: 100%;'; 
		}

I'm not too sure why we deduct spacing from the width anyway (it seems to me it should just be the space between the email field and button in this case) so I may be misunderstanding the original code.

What do you think about this?

Copy link
Contributor

@coder-karen coder-karen Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Actually that's probably better for a follow-up instead of here since it isn't really related to the bug. I'll make sure this current fix gets in to the next release regardless.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good question. I'm not sure what the best fix is. Maybe we should be changing the default spacing value to 0?

We can see about fixing this in a follow-up for sure. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@coder-karen @jeherve Hi! where is this follow up happening? @obenland let me know that some of our patterns look broken because of the not quite 100% width button.

For example,

Copy link
Member Author

Choose a reason for hiding this comment

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

@iamtakashi We didn't create any other PR as we weren't sure about the best approach to take. Do you have any opinion on the above?

: 'width: 100%;';
}

Expand Down