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

Table block: Make figcaption styles consistent between editor and front end #46172

Merged
merged 4 commits into from
Dec 1, 2022
Merged

Table block: Make figcaption styles consistent between editor and front end #46172

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c5a7063
Move figcaption styles to style.scss
mikachan Nov 29, 2022
d47bb82
Remove theme mixin from style.scss
mikachan Nov 30, 2022
1c33253
Increase font size percentage
mikachan Nov 30, 2022
6671cba
Remove figcaption styles from style, keep them in theme
mikachan Nov 30, 2022
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
4 changes: 0 additions & 4 deletions packages/block-library/src/table/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,6 @@
border-style: double;
}

figcaption {
@include caption-style-theme();
}

// This is only required in the editor to overcome the fact the editor
// rewrites border width styles into shorthand.
table.has-individual-borders {
Expand Down
7 changes: 7 additions & 0 deletions packages/block-library/src/table/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@
padding: 0.5em;
}

// Match caption styles across front end and editor
figcaption {
@include caption-style-theme();
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems there is a convention for mixins with the suffix -theme to only use them in the theme styles — is it an issue to create an inconsistency here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so yes :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks both, that makes sense. I've removed the use of this mixin here and instead used specific styles. I went with:

	figcaption {
		color: inherit;
		font-size: max(13px, 80%);
		text-align: center;
	}

The only difference here is that the font size is set to a minimum of 13px, as this is the same size that was set in the mixin, but then I've also allowed it to be 80% of the font size set by the theme. I'm not sure if this is too opinionated, but I thought it would allow for a more theme-related font size.


color: inherit;
}

// Fixed layout toggle
.has-fixed-layout {
table-layout: fixed;
Expand Down
4 changes: 0 additions & 4 deletions packages/block-library/src/table/theme.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,4 @@
th {
word-break: normal;
}

figcaption {
@include caption-style-theme();
}
}