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.

Sign up for GitHub

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

File block: Add spacing support #45107

Merged
merged 15 commits into from
Aug 4, 2023
Merged

File block: Add spacing support #45107

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
017af05
File block: Add spacing support
carolinan Oct 19, 2022
90d6346
Merge branch 'trunk' into update/file-spacing-support
carolinan Mar 6, 2023
cf727a8
Merge branch 'trunk' into update/file-spacing-support
carolinan Mar 14, 2023
ec8bbe6
tentatively add margin to classic.scss
carolinan Mar 14, 2023
6c59b40
Update classic.scss
carolinan Mar 14, 2023
39b239d
Merge branch 'trunk' into update/file-spacing-support
carolinan May 20, 2023
f4b6ecd
Merge branch 'trunk' into update/file-spacing-support
carolinan May 25, 2023
dcf14cd
Merge branch 'trunk' into update/file-spacing-support
carolinan Jun 14, 2023
460905d
Merge branch 'trunk' into update/file-spacing-support
carolinan Jun 26, 2023
fd1d087
Merge branch 'trunk' into update/file-spacing-support
carolinan Jun 30, 2023
137786d
Merge branch 'trunk' into update/file-spacing-support
carolinan Jul 2, 2023
9256bb8
Merge branch 'trunk' into update/file-spacing-support
carolinan Jul 11, 2023
3b55fc6
Merge branch 'trunk' into update/file-spacing-support
carolinan Jul 21, 2023
7035826
Merge branch 'trunk' into update/file-spacing-support
carolinan Jul 24, 2023
b8bb5c2
Remove experimental bottom margin
carolinan Jul 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/reference-guides/core-blocks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ Add a link to a downloadable file. ([Source](https://github.com/WordPress/gutenb

- **Name:** core/file
- **Category:** media
- **Supports:** align, anchor, color (background, gradients, link, ~~text~~)
- **Supports:** align, anchor, color (background, gradients, link, ~~text~~), spacing (margin, padding)
- **Attributes:** displayPreview, downloadButtonText, fileId, fileName, href, id, previewHeight, showDownloadButton, textLinkHref, textLinkTarget

## Classic
Expand Down
11 changes: 11 additions & 0 deletions packages/block-library/src/file/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,17 @@
"supports": {
"anchor": true,
"align": true,
"spacing": {
"margin": true,
"padding": true
},
"__experimentalStyle": {
"spacing": {
"margin": {
"bottom": "1.5em"
}
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, I don't think we can make this switch yet as it would be a regression for themes without a theme.json file due to the generated styles not being loaded for them.

See: #46818

Copy link
Contributor

@andrewserong andrewserong Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for mentioning this, and for linking through to the WIP work to fix layout conflicts with margins. Something that's also relevant here is that in the exploration by @tellthemachines over in #47858 the Columns block's default margin-bottom rule has its specificity reduced. The idea in that PR is that for layout rules to be able to have reduced specificity, we likely will also need all core blocks' default margin rules to be lower specificity too (e.g. wrapped in a :where() rule) so that low specificity layout rules can easily override it. I imagine we'd need to do that for the file block, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I don't think we can make this switch yet as it would be a regression for themes without a theme.json file due to the generated styles not being loaded for them.

There is a specific stylesheet loaded for them now though? The style should be added there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't aware there was. I was basing my comment off both #46818 and #45198 which are still open indicating the issue remains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is where the classic.scss file was introduced.
#44334

https://developer.wordpress.org/reference/functions/wp_enqueue_classic_theme_styles/
In 6.2 it uses wp_theme_has_theme_json() to determine when the file is loaded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for trying out the classic.scss approach @carolinan

Unfortunately, I think things have changed a bit recently. #47858 landed which fixed the layout margin rule specificity. Part of that meant lowering the specificity of some blocks' CSS rules. This included the File block.

The current state of this PR means we would have the margin rule in 3 places (style.scss, classic.scss, and block.json).

I suspect we might be better off removing the addition of the new rule to classic.scss and the block.json via __experimentalStyle. The latter overrides the lower margin specificity required to work alongside layout supports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the layout margin rules are fixed, there are issues with two of the other blocks I have spacing PR's for.

Copy link
Contributor

Choose a reason for hiding this comment

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

The "fixing" of margin rules was so that a block's global styles for margin could be applied. I believe the fix from #47858 is still working. It sounds like the interaction between the overall layout styles and top-level block margins is what you don't think is working correctly?

I'm about to start looking more closely at the Preformatted spacing PR.

The left/right margins not being applied for top-level blocks makes sense but I need to have a play around with the PR to see what's happening on the frontend. I'll comment further on that PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Coming back to this one with fresh eyes, I still think we just need to remove the classic.scss style and this __experimentalStyle entry and we should be able to move ahead with this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed 👍

"color": {
"gradients": true,
"link": true,
Expand Down
3 changes: 2 additions & 1 deletion packages/block-library/src/file/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
.wp-block-file {
margin-bottom: 1.5em;
// This block has customizable padding, border-box makes that more predictable.
box-sizing: border-box;

&:not(.wp-element-button) {
font-size: 0.8em;
Expand Down