Remove margins from Notice component

[ecgan]

What?

Closes issue #54741.

In this PR, we remove the margins from the Notice component, so that developers using the component does not need to manually override the margins.

Here's a screenshot of the Notice components nested within NoticeList component, retrieved from http://localhost:50240/?path=/story/components-notice--notice-list-subcomponent after running npm run storybook:dev:

Why?

See issue #54741.

How?

By removing the margins in the Notice component CSS.

Testing Instructions

1. Run npm install and then npm run storybook:dev
2. View the Notice components in storybook page.

Testing Instructions for Keyboard

Screenshots or screencast

[jorgefilipecosta]

LGTM 👍

[ecgan]

@jorgefilipecosta , thanks for the approval!

There is one question that I would like to ask: since this change might impact the existing users of the Notice component, how will we release and communicate this change? e.g. Will this be in the @wordpress/components major version bump? Will we mention this possible breaking change in changelog or blog post etc? 🤔

Also, I'm not authorized to merge this PR. It says "The base branch restricts merging to authorized users." Could you help to merge this PR please?

[ecgan]

Also, I see we have some failed required checks here. I don't think they are related to my changes here. I want to re-run them but I don't see the re-run buttons. I think I may not have the permission to re-run the jobs. Could you also help with those please?

[youknowriad]

Sorry this fell through the cracks. Can you try to rebase the branch to see if the test errors go with the latest trunk updates?

[ecgan]

@youknowriad , thanks for the suggestion. I merged the latest trunk branch into my branch. Now there is one required e2e test that is failing. How should we proceed? 🤔

[youknowriad]

I've restarted the failing e2e test to see whether it's an intermittent failure. On the other hand, it would be great to update the CHANGELOG of the components package to mention the change maybe as ### Enhancements under the ## unreleased version.

[ecgan]

Hi @youknowriad ,

I've restarted the failing e2e test to see whether it's an intermittent failure.

Thank you. It got passed.

On the other hand, it would be great to update the CHANGELOG of the components package to mention the change maybe as ### Enhancements under the ## unreleased version.

Done in aa91ab3 (#54800).

If everything is alright, could you or @jorgefilipecosta help to merge this PR (I am not authorized to merge PR)?

[youknowriad]

cc @ciampo just in case.

[ciampo]

Thank you @ecgan for working on this , and @youknowriad and @jorgefilipecosta for reviewing!

This change is definitely a good change — in general, to ensure good layout composition, components should not set any properties affecting their own size or margins.

We've been always working on removing margins from control components, and we've usually adopted a more careful approach (ie. adding a transitional prop to make the change more gradual), but I don't think that this should cause much disruption.

[ciampo]

@ecgan , I think it would be great to have a small follow-up cleaning any related style overrides (that now are not necessary anymore) like the ones listed in #54800 (comment).

We could look for .components-notice and see what we find.

[ecgan]

@ciampo , thank you for the comments. 🙏 🙂

we've usually adopted a more careful approach (ie. adding a transitional prop to make the change more gradual)

Could you share a bit more info or examples on how we can do that for this Notice case in this PR? I thought about being more careful and being backward compatible, but couldn't think of a good idea.

Do you mean something like <Notice removeMargins />, using removeMargins props to programmatically remove the margins? I'm thinking it may not be a good idea, because users would have to specify the prop, and then remove the prop in the future when it is no longer necessary. Or maybe you mean something else? 🤔

I think it would be great to have a small follow-up cleaning any related style overrides (that now are not necessary anymore) like the ones listed in #54800 (comment).

We could look for .components-notice and see what we find.

That's a good idea. I don't actively work in this Gutenberg repo, and I will have to be AFK for some time soon, but I would be happy to contribute. I'll try to come up with something. 🙂

[ciampo]

Could you share a bit more info or examples on how we can do that for this Notice case in this PR? I thought about being more careful and being backward compatible, but couldn't think of a good idea.

You can see this project as an example — the idea is to introduce a temporary prop, which will allow us to gradually transition to the new styles.

That's a good idea. I don't actively work in this Gutenberg repo, and I will have to be AFK for some time soon, but I would be happy to contribute. I'll try to come up with something. 🙂

That would be great, thank you!

[ecgan]

@ciampo ,

You can see this project as an example — the idea is to introduce a temporary prop, which will allow us to gradually transition to the new styles.

Thanks for the link! From the link, I found the guidelines, which states:

DOES need formal deprecation

• Removing an outer margin.

... which is not being followed in this PR. 😅

I guess we will take your words in #54800 (comment) ("but I don't think that this should cause much disruption.") for it and move forward.

If we want to stick with the above guideline, I can revert this PR, and come up with another PR with deprecation. It may be a good learning opportunity for me too. Just let me know if I should do this.

I'll try to come up with something. 🙂

That would be great, thank you!

I have created issue #55279 as a follow up to clean up unneeded style overrides for Notice component.

[ciampo]

I guess we will take your words in #54800 (comment) ("but I don't think that this should cause much disruption.") for it and move forward.

@ecgan , let's move forward this time, and keep eyes open for any potential regression. In case anything comes up, we can always go with the plan that you suggested:

I can revert this PR, and come up with another PR with deprecation. It may be a good learning opportunity for me too. Just let me know if I should do this.