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

Mailchimp Block: Fix styling issue with FSE #19566

Merged
merged 6 commits into from
Apr 27, 2021
Merged

Mailchimp Block: Fix styling issue with FSE #19566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
738ffa5
[not verified] Add base styles for mailchimp form
stacimc Apr 19, 2021
be158e0
[not verified] Add changelog
stacimc Apr 19, 2021
86bbbdb
[not verified] Remove base styles for button
stacimc Apr 21, 2021
af9ee49
[not verified] Remove input padding to prevent overriding theme styles
stacimc Apr 21, 2021
3d5aa34
[not verified] Add margin back to bottom of button
stacimc Apr 21, 2021
c645b05
[not verified] Add box-sizing so that the width doesn't overflow the …
andrewserong Apr 23, 2021
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-mailchimp-styling-for-fse
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: compat

Mailchimp block: add base styles to ensure compatibility with WordPress' Full Site Editing feature.
7 changes: 6 additions & 1 deletion projects/plugins/jetpack/extensions/blocks/mailchimp/view.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,15 @@
}
}

.wp-block-jetpack-button {
.wp-block-jetpack-button, p {
margin-bottom: 1em;
}

input {
Copy link
Member

Choose a reason for hiding this comment

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

This looks good to me visually, but I can see that it's overriding the theme provided styles when using the Barnsbury theme in wpcom. Without the change applied, the theme's CSS targets the input field and sets it to 16px padding:

image

With the change applied, this CSS overrides it and sets the padding to 14px:

image

With this particular theme the difference isn't all that noticeable, but I imagine other themes might diverge a bit more in their padding? That said, I can't quite think how we'd reduce the specificity here to allow the theme styles to override it 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I'd prefer to not override the themes here. Maybe this particular case is a reasonable one to defer -- remove the styling here and instead ask the block themes to provide a better base style?

Copy link
Member

@andrewserong andrewserong Apr 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good point, I notice that the Search block in core also doesn't apply padding by default in TT1-blocks, so maybe it's better to leave it (aside from the width: 100% that you've added).

Search block:

image

Unfortunately I still haven't had much luck investigating the placeholder issue in my environment. I'll try again tomorrow, as I'd like to get to the bottom of what's causing it. Apologies for holding up the review! 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, thank you!

Copy link
Member

Choose a reason for hiding this comment

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

It turns out the weird placeholder issue I encountered occurs only when you view the front end of the site while logged in, with a site that doesn't have a paid plan, and you also have a block on the page that displays an upgrade nudge. I've put together a fix in: #19639

box-sizing: border-box;
width: 100%;
}

.error,
.error:focus {
outline: 1px;
Expand Down