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

A margin option is added to the Embeded preview block #39384

Merged

Conversation

HILAYTRIVEDI
Copy link
Contributor

What?

This PR will add the option to set margins to the embeds.

Why?

This PR will allow the user to add the margins to the embeds preview.

How?

The option is added to the inspector control to add the margins to the block.

Testing Instructions

Here are some steps to check the implementation ;

  1. Add the embedded block to posts or pages.
  2. Add the link to the embed.
  3. After adding the link you will notice the additional option to set the margin to the preview.

Screenshots or screencast

https://www.loom.com/share/6dbca6117ec6416aad995319736db226

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Mar 11, 2022
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @HILAYTRIVEDI! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@carolinan
Copy link
Contributor

Thank you for the pull request @HILAYTRIVEDI.

Gutenberg already has a Dimensions panel with a margin option.
Instead of a new attribute and a new default margin (which would change the design of the block), we should reuse the existing panel.

Please add block support for spacing and margin in block.json instead.
Example: At the bottom of block.json of the embed block, you will find a section for supports:

	"supports": {
		"align": true
	},

You can add the block support for spacing and margin there:

	"supports": {
		"align": true,
		"spacing": {
			"margin": true
		}
	},

This is a much smaller change, and you do need to regenerate the documentation, but you don't need to make the changes to edit.js, save.js, the controls or the preview.

@skorasaurus skorasaurus added the [Block] Embed Affects the Embed Block label Mar 11, 2022
@HILAYTRIVEDI
Copy link
Contributor Author

Hi @carolinan, Thanks for this. I reverted my changes and added the margin support to the block.json file as per your instructions.

@carolinan
Copy link
Contributor

Hi @HILAYTRIVEDI Are you able to continue working on this pull request? (to rebase it and solve the conflict).

@HILAYTRIVEDI
Copy link
Contributor Author

Hi @carolinan yes sure I will do that.

@HILAYTRIVEDI
Copy link
Contributor Author

Hi @carolinan, I can't see any conflicts here, all good to go from my end.

@carolinan carolinan added the [Type] Enhancement A suggestion for improvement. label Oct 13, 2022
Copy link
Contributor

@carolinan carolinan left a comment

Choose a reason for hiding this comment

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

I have tested the pull request with Twenty Twenty-Three, which has support for spacing enabled in theme.json.
I tested a youtube embed and an embedded link to a WordPress.org support article and the margin worked well in the editor and front.

(In post content, the margin is less visible because of the layout constraints)

@HILAYTRIVEDI
Copy link
Contributor Author

Hi @carolinan, can we merge this if all looks good?

@carolinan
Copy link
Contributor

The PR needs to be rebased so that all tests are run.

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this one @HILAYTRIVEDI 👍

The change looks good to me and tests as advertised.

In terms of the padding block support it makes sense to omit it. It's arguable as to whether it would provide any benefit, even if we opted into background color support. Adding the ability to set a border on the embed might be more desirable in a follow-up.

So given that, I think this should be right to go after it gets a rebase and the tests pass.

@HILAYTRIVEDI
Copy link
Contributor Author

Okay I will rebase it by EOD (IST).

@HILAYTRIVEDI
Copy link
Contributor Author

HILAYTRIVEDI commented Apr 7, 2023

Hi @carolinan and @aaronrobertshaw Can we merge this? As it passed all the checks.

@aaronrobertshaw aaronrobertshaw merged commit 5981e7e into WordPress:trunk Apr 10, 2023
@github-actions github-actions bot added this to the Gutenberg 15.6 milestone Apr 10, 2023
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Embed Affects the Embed Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Needs User Documentation Needs new user documentation [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants