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

File block: Add spacing support #45107

Merged
merged 15 commits into from Aug 4, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/reference-guides/core-blocks.md
Expand Up @@ -266,7 +266,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

## Footnotes
Expand Down
4 changes: 4 additions & 0 deletions packages/block-library/src/classic.scss
Expand Up @@ -14,6 +14,10 @@
font-size: 1.125em;
}

.wp-block-file {
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.

As noted in the __experimentalStyle thread, we'll need to remove this style from classic.scss to maintain the lower specificity default set in style.scss required by layout supports.

.wp-block-file__button {
background: #32373c;
color: $white;
Expand Down
11 changes: 11 additions & 0 deletions packages/block-library/src/file/block.json
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

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: 3 additions & 0 deletions packages/block-library/src/file/style.scss
@@ -1,4 +1,7 @@
.wp-block-file {
// 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