-
Notifications
You must be signed in to change notification settings - Fork 798
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
Mailchimp Block: Fix styling issue with FSE #19566
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
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 🤖 Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
7bef3cf
to
5b53683
Compare
Unfortunately I haven't been able to get this one to work properly in wpcom. I think it's mostly unrelated to your PR, but on the front end of the site, the Mailchimp block appears to be getting rendered with the Also, the rendered button has the I'm not sure if I've done something strange to my testing environment, or if this is particular to wpcom? |
margin-bottom: 1em; | ||
} | ||
|
||
input { |
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.
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:
With the change applied, this CSS overrides it and sets the padding to 14px
:
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 🤔
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.
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?
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.
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:
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! 🤔
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.
No worries, thank you!
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.
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
@andrewserong Hm, I'm not seeing the styling issues you mentioned; maybe a bug with the placeholder class not getting removed correctly after you configured the Mailchimp account?
I did notice after rebasing that I'm now seeing |
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.
Thanks for working on this @stacimc! I got to the bottom of the placeholder issue, which will be fixed by #19639.
With that applied, I noticed that the width: 100%
still overflows the container slightly in TT1-blocks, so I've pushed a tiny change to add in box-sizing: border-box
which gets it playing nicely in that theme.
Before | After |
---|---|
This LGTM now! I also tested that the margins look good on the front-end of existing themes. The updated margin looks better to me, less of a gap between the input field and the button 👍
The base styles should instead come from the jetpack button (#19595)
ea89d19
to
c645b05
Compare
Rebased to pass status checks |
Great news! One last step: head over to your WordPress.com diff, D60371-code, and commit it. Thank you! |
r225216 |
Fixes #19481
Changes proposed in this Pull Request:
Notes
Screenshots
Mailchimp block in the footer of a FSE site using TT1 blocks
Mailchimp block in the post content of a site using TT1
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Sync this PR; if you would like to see the final styling of the button, you should also apply the #19595 PR.