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

Archeo: Try using button as an element #5849

Merged
merged 8 commits into from
Jul 5, 2022
Merged

Archeo: Try using button as an element #5849

merged 8 commits into from
Jul 5, 2022

Conversation

scruffian
Copy link
Member

This is useful for testing https://github.com/WordPress/gutenberg/pull/40260/files

Things to note:

  • The CSS for the search block button has been removed
  • The styles for buttons have been moved from styles > blocks > core/button to styles > elements > button.

@MaggieCabrera
Copy link
Contributor

We can merge this when 13.4 is live

@MaggieCabrera MaggieCabrera added [Theme] Archeo Automatically generated label for Archeo. Do NOT merge! labels May 26, 2022
@MaggieCabrera
Copy link
Contributor

There's a bit of a change before and after this PR, I think we may want to adjust it a little bit

Before:

Editor Frontend
Screenshot 2022-05-26 at 09 28 29 Screenshot 2022-05-26 at 09 28 24

After:

Editor Frontend
Screenshot 2022-05-26 at 09 26 50 Screenshot 2022-05-26 at 09 26 56

@MaggieCabrera
Copy link
Contributor

@scruffian I added a few changes to make the buttons look like they should. The padding control for the search block button can only be done via the elements API or CSS, since the search block padding is not affecting the button, but the whole block. By assigning a padding to the button element, the outline variation is always bigger than the regular button is, so that required extra CSS to fix. What do you think? If you agree with that, I'll go ahead and replicate this on the rest of the themes.

@MaggieCabrera MaggieCabrera self-assigned this May 26, 2022
@scruffian
Copy link
Member Author

I'd rather we try to find ways to remove the CSS altogether. Are there changes we can make to Gutenberg so that we don't need it?

@MaggieCabrera
Copy link
Contributor

I'd rather we try to find ways to remove the CSS altogether. Are there changes we can make to Gutenberg so that we don't need it?

if we could target block variations from theme.json... Can't think of anything else

@MaggieCabrera
Copy link
Contributor

I'd rather we try to find ways to remove the CSS altogether. Are there changes we can make to Gutenberg so that we don't need it?

if we could target block variations from theme.json... Can't think of anything else

WordPress/gutenberg#27476 I believe would be the issue for this

@scruffian
Copy link
Member Author

Ok that looks like it will be a while away yet then, but moving closer :)

For now this seems fine...

Copy link
Contributor

@MaggieCabrera MaggieCabrera left a comment

Choose a reason for hiding this comment

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

Alright then! I'll approve this and bring this in once dotcom gets the GB change. I'll work on a PR for Blockbase next.

@MaggieCabrera
Copy link
Contributor

Ok this one is not done, it looks like we didn't remove the file button CSS since WordPress/gutenberg#41239 was merged, but that has bugs too.

@MaggieCabrera
Copy link
Contributor

MaggieCabrera commented Jun 8, 2022

the bugs should be fixed in WordPress/gutenberg#41393 That makes 13.5 our merging target for this PR

@MaggieCabrera
Copy link
Contributor

We should make a quick test on dotcom but this should be mergeable now

@pbking
Copy link
Contributor

pbking commented Jul 5, 2022

Tested on wpcom.
Works like a charm. :)
Sending it out into the wild now.

@pbking pbking merged commit 87f535a into trunk Jul 5, 2022
@mikachan mikachan deleted the try/button-element branch July 6, 2022 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Theme] Archeo Automatically generated label for Archeo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants