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

Blockbase: move button padding styles from ponyfill to theme.json #5901

Closed
wants to merge 7 commits into from

Conversation

madhusudhand
Copy link
Member

@madhusudhand madhusudhand commented Apr 21, 2022

Changes proposed in this Pull Request:

Moved core/button block padding styles from ponyfill to theme.json so that global styles can be configured.

Screenshots

Editor:

image

Frontend:

button-global-styles

Related issue(s):

Fixes: #5856
Related: #5847

@scruffian
Copy link
Member

My main concern with this is that the search and file blocks also match the same button style. I'm trying to introduce a button element block to help deal with that: WordPress/gutenberg#40260

@pbking
Copy link
Contributor

pbking commented Apr 22, 2022

Funny, I thought I had left comments on this already.

I also wanted to note that the 'outline' styles of the button don't follow the padding rules, however leveraging styling buttons "The Blockbase Way" does. Here is an example from this branch (with padding set to 35 px):
image

Also this behavior won't work as expected in any of the Blockbase children. This is Quadrat in regular then hover:

image

image

That's in addition to the "other non-block buttons" that @scruffian mentions. I do think that the button element could definitely help that out, but we also have the hover state management that needs to be dealt with, as well as rolling those details into block styles (such as the outline style) that needs to be figured out and dealt with as well before we can move on from this method of button styling in Blockbase.

@madhusudhand
Copy link
Member Author

@pbking addressed the issues in blockbase and in the child themes.
To verify the approach, committed changes related to single child theme quadrat.

All other themes needs to be compiled and verified.

Blockbase
blockbase-btn

Quadrat
quadrat-btn

@madhusudhand
Copy link
Member Author

madhusudhand commented Apr 25, 2022

Similar to other block themes, I moved the button hover styles to blockbase/style.css in blockbase.

However, these styles are not getting applied to child themes such as quadrat
Updated blockbase/functions.php, but still it doesn't work.
What am I missing here?

@madhusudhand madhusudhand force-pushed the blockbase-btn-global-styles branch from 2a01ba9 to 456e5d1 Compare April 25, 2022 11:58
@pbking
Copy link
Contributor

pbking commented Apr 26, 2022

Similar to other block themes, I moved the button hover styles to blockbase/style.css in blockbase.

Anything that Blockbase needs to make available to child themes should still reside in ponyfill.css. The only change we should be making to support this is what css is in ponyfill, not where it is.

@pbking
Copy link
Contributor

pbking commented Apr 26, 2022

All other themes needs to be compiled and verified.

Any changes that require child themes to change isn't a change we can make for Blockbase. There are an unknown number of themes outside of Automattic (and even inside Automattic not under our management) that would need to change. So any changes we make to Blockbase (at this point) should not require changes to child themes.
That does limit what changes we can make, for sure, but that's the cost of being a "parent theme" I think.

@madhusudhand madhusudhand marked this pull request as draft June 2, 2022 07:54
@pbking pbking marked this pull request as ready for review July 13, 2022 17:12
@pbking
Copy link
Contributor

pbking commented Jul 13, 2022

@madhusudhand I took a look at this and I'm not sure why I was opposed earlier. The changes to the child themes are reasonable. I'm knocking this branch about and it's the best fix. Thank you for working on this.

I'll push the change rebuilding the rest of the child themes css.

@pbking pbking added this to the Blockbase 3.0 milestone Jul 13, 2022
@pbking pbking force-pushed the blockbase-btn-global-styles branch from d763b10 to 0a80ee5 Compare July 13, 2022 17:41
pbking added a commit that referenced this pull request Jul 20, 2022
Refactor/blockbase color admin (#6043)
Moved templates from old folder location to new (#6073)
Blockbase: Implement the Button elements API (#6041)
Blockbase: Implement Comment Block and removed CSS (#6080)
Fix/migrate blockbase font self hosted (#6123)
Blockbase children: update comments block (#6153)
Blockbase: Changed the trigger to render social icons (#6079)
Blockbase: move button padding styles from ponyfill to theme.json (#5901)

Co-authored-by: Grant Kinney <[email protected]>
Co-authored-by: Jeremy Yip <[email protected]>
Co-authored-by: MaggieCabrera <[email protected]>
Co-authored-by: madhusudhand <[email protected]>
@pbking pbking mentioned this pull request Jul 20, 2022
@pbking
Copy link
Contributor

pbking commented Jul 20, 2022

Closing: Work merged in #6167

@pbking pbking closed this Jul 20, 2022
@madhusudhand madhusudhand deleted the blockbase-btn-global-styles branch July 21, 2022 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blockbase: Button block does not respect paddings defined in Global Styles
3 participants