Calendar Block: Add color supports and polish styles #42029
Conversation
t-hamano
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
| "align": true, | ||
| "color": { | ||
| "link": true, | ||
| "background": false, |
There was a problem hiding this comment.
The table block supports background color, should it also support it in the calendar block?
In my opinion, I think support is unnecessary.
There was a problem hiding this comment.
I think it should be enabled because it would give everyone more design options.
There was a problem hiding this comment.
Sorry, late to the party. I had been toying around with adding color support for the calendar block over in #43299
I'll close my PR in favour of this one.
Just wanted to mention something I discovered in relation to background color support:
When selecting a preset color, followed by a custom color, the block doesn't rerender.
I've pinpointed it to the use of useDebounce in the ServerSideRender component.
The debounced function is called, but it doesn't fire. I suspect that it's cancelled too early here.
It's a strange equality check bug that I haven't worked out yet.
I tested on this branch:
2022-08-19.11.57.12.mp4
It occurs every so often, but not all the time so might be acceptable.
But if I enable"gradients": true in color supports, the effect is more pronounced:
2022-08-19.12.08.58.mp4
Lesson: don't enable "gradient" for now 😄
|
Size Change: +4.75 kB (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
|
@WordPress/block-themers Any feedback on this calendar block PR? |
|
Pinging @markhowellsmead as well in case you want to review this CSS change. |
draganescu
changed the title
draganescu
added
[Type] Enhancement
I am concerned about the loss of the background color too. Maybe we can scope this back to changes which won't have a significant impact on existing blocks? I also think the calendar caption should have a |
|
I tried to support background but hit a wall. First, if we simply opt for background with block support, the style will be applied to the block itself and will look like this:
But most people would expect the background color to be applied to the table tag inside, just as the table block is: In this case, If a link color is applied in a static block,
However, this process is not executed on dynamic blocks that support link colors and have skipped serialization. Therefore, we need to generate the class name and inline styles within the Does anyone have a better way to do this? 🙏 |
|
Please also remove the hard-coded |
|
@t-hamano the way you described your approaches sounds pretty good to me! |
In #43299 I rather inelegantly got around this using There's probably a better way than my hack though. |
|
I have added the following brush ups: Backward compatibility for the header background colorIf both text and background color are not set, the previous gray header background color is retained. gutenberg/packages/block-library/src/calendar/style.scss Lines 10 to 18 in aaa96dc Background color supportI think text and background colors should only apply to tables, just as table blocks do. The code is based on the search block, but if the HTML Walker class is adopted, it will be simpler to write. Remove text-decoration style
About skip serialization
I had assumed that However, I learned that Screencasta23c0c68be0f3e3d682519554c2fb8e3.mp4 |
Regarding the above proposal, I would like to push an additional issue and PR if this PR is merged. |
|
Thank you for your kind advice, @aaronrobertshaw !
a55779cb19f31f7c852efe320847f54c.mp4Generally works as expected, but there is a problem with the gray header background color for backward compatibility. |
There was a problem hiding this comment.
Thanks for the continued effort @t-hamano 🙇
I think this PR is really starting to take shape!
After reviewing the latest changes, it appears as though we are currently split only half using the style engine, i.e. for inline styles only. I think being consistent and using it for all the block support generated styles would be beneficial and automatically pick up color support changes in the future.
I've added a diff below that updates the use of the style engine, among other fixes, and I think this cleans things up a little.
Generally works as expected, but there is a problem with the gray header background color for backward compatibility.
When a color is applied via global style, has-text-color and has-background classes aren't applied to the table, so the gray background color is retained.
I think the only way to solve this is to remove this backward compatibility, what do you think?
Luckily, I don't think that is the only way to solve this. We could target the th with the feature-level selectors as well so that they'll trump the :where rule you've already added. Then with a few extra tweaks to the styles, we can smooth out the consequences for individual blocks. I also noticed a bug in that setting a text color prevented the default background from applying.
I've included the suggestions above in the final diff below. The demo video below is the result I get when applying the diff.
Diff illustrating suggestions
diff --git a/packages/block-library/src/calendar/block.json b/packages/block-library/src/calendar/block.json
index 747bfd67c8..d1f2e0b604 100644
--- a/packages/block-library/src/calendar/block.json
+++ b/packages/block-library/src/calendar/block.json
@@ -24,7 +24,7 @@
"background": true,
"text": true
},
- "__experimentalSelector": "table"
+ "__experimentalSelector": "table, table th"
},
"typography": {
"fontSize": true,
diff --git a/packages/block-library/src/calendar/editor.scss b/packages/block-library/src/calendar/editor.scss
new file mode 100644
index 0000000000..b12256cb74
--- /dev/null
+++ b/packages/block-library/src/calendar/editor.scss
@@ -0,0 +1,13 @@
+.editor-styles-wrapper {
+ .wp-block-calendar {
+ table {
+ &.has-background th {
+ background-color: inherit;
+ }
+
+ &.has-text-color th {
+ color: inherit;
+ }
+ }
+ }
+}
diff --git a/packages/block-library/src/calendar/index.php b/packages/block-library/src/calendar/index.php
index eac45b486b..c3b89f67e8 100644
--- a/packages/block-library/src/calendar/index.php
+++ b/packages/block-library/src/calendar/index.php
@@ -40,29 +40,25 @@ function render_block_core_calendar( $attributes ) {
}
}
- $color_classes = array();
+ $color_block_styles = array();
- if ( ! empty( $attributes['textColor'] ) ) {
- $color_classes[] = sprintf( 'has-text-color has-%s-color', $attributes['textColor'] );
- }
- if ( ! empty( $attributes['backgroundColor'] ) ) {
- $color_classes[] = sprintf( 'has-background has-%s-background-color', $attributes['backgroundColor'] );
- }
+ // Text color.
+ $preset_text_color = array_key_exists( 'textColor', $attributes ) ? "var:preset|color|{$attributes['textColor']}" : null;
+ $custom_text_color = _wp_array_get( $attributes, array( 'style', 'color', 'text' ), null );
+ $color_block_styles['text'] = $preset_text_color ? $preset_text_color : $custom_text_color;
- $inline_styles = '';
+ // Background Color.
+ $preset_background_color = array_key_exists( 'backgroundColor', $attributes ) ? "var:preset|color|{$attributes['backgroundColor']}" : null;
+ $custom_background_color = _wp_array_get( $attributes, array( 'style', 'color', 'background' ), null );
+ $color_block_styles['background'] = $preset_background_color ? $preset_background_color : $custom_background_color;
- if ( ! empty( $attributes['style'] ) ) {
- $styles = gutenberg_style_engine_get_styles( $attributes['style'] );
- if ( ! empty( $styles['css'] ) ) {
- $inline_styles = " style=\"${styles['css']}\"";
- }
- if ( ! empty( $styles['classnames'] ) ) {
- $color_classes[] = $styles['classnames'];
- }
- }
+ // Generate color styles and classes.
+ $styles = gutenberg_style_engine_get_styles( array( 'color' => $color_block_styles ), array( 'convert_vars_to_classnames' => true ) );
+ $inline_styles = empty( $styles['css'] ) ? '' : sprintf( ' style="%s"', esc_attr( $styles['css'] ) );
- $calendar = str_replace( '<table', '<table' . $inline_styles, get_calendar( true, false ) );
- $calendar = str_replace( 'class="wp-calendar-table', 'class="wp-calendar-table ' . implode( ' ', $color_classes ), $calendar );
+ // Apply color classes and styles to the calendar.
+ $calendar = str_replace( '<table', '<table' . $inline_styles, get_calendar( true, false ) );
+ $calendar = str_replace( 'class="wp-calendar-table', 'class="wp-calendar-table ' . esc_attr( $styles['classnames'] ), $calendar );
$wrapper_attributes = get_block_wrapper_attributes();
$output = sprintf(
diff --git a/packages/block-library/src/calendar/style.scss b/packages/block-library/src/calendar/style.scss
index 3095e3f50a..f28900c0ae 100644
--- a/packages/block-library/src/calendar/style.scss
+++ b/packages/block-library/src/calendar/style.scss
@@ -12,8 +12,16 @@
border-collapse: collapse;
// Keep the hard-coded header background color for backward compatibility.
- &:where(:not(.has-text-color):not(.has-background)) th {
+ &:where(:not(.has-background)) th {
background: $gray-300;
}
+
+ &.has-background th {
+ background-color: inherit;
+ }
+
+ &.has-text-color th {
+ color: inherit;
+ }
}
}
diff --git a/packages/block-library/src/editor.scss b/packages/block-library/src/editor.scss
index edb66416af..3af72b2448 100644
--- a/packages/block-library/src/editor.scss
+++ b/packages/block-library/src/editor.scss
@@ -4,6 +4,7 @@
@import "./block/editor.scss";
@import "./button/editor.scss";
@import "./buttons/editor.scss";
+@import "./calendar/editor.scss";
@import "./categories/editor.scss";
@import "./code/editor.scss";
@import "./columns/editor.scss";
Demo video
Screen.Recording.2022-09-12.at.7.22.34.pm.mp4
Lastly, is there anything else we can do to maintain backwards compatibility for the other default styles removed in this PR? I've run out of time for today to digger any deeper sorry.
Hope this helps; good luck!
|
I appreciate your detailed advice, @aaronrobertshaw! Again, I would like to summarize the specifications of the calendar block in The calendar block in the trunk
The behavior expected by this PR
|
There was a problem hiding this comment.
I appreciate your patience with this PR @t-hamano. 🙇
Unfortunately, the latest changes adding more backwards compatibility, removing some of the previously suggested CSS rules, and including th and caption in the feature level selector have resulted in a few regressions.
During my latest round of testing, the link color still functions well, but there were issues with the background and text colors, as well as the caption.
- Global Styles set background colors don't override the default
thbackground on the frontend - Individual block text and background colors now don't override the Global Styles set values
- The background color doesn't get applied to the caption when setting it only for individual blocks
After some more fiddling with our combination of feature level selectors and styles I believe I have it working and ticking all the boxes you outlined in your last comment. The suggested diff is included below.
Click for suggested changes
diff --git a/packages/block-library/src/calendar/block.json b/packages/block-library/src/calendar/block.json
index 89afb26410..2accd7142c 100644
--- a/packages/block-library/src/calendar/block.json
+++ b/packages/block-library/src/calendar/block.json
@@ -24,7 +24,7 @@
"background": true,
"text": true
},
- "__experimentalSelector": "table, th, td, caption"
+ "__experimentalSelector": "table, th"
},
"typography": {
"fontSize": true,
diff --git a/packages/block-library/src/calendar/style.scss b/packages/block-library/src/calendar/style.scss
index ddeb10d421..969d1aafec 100644
--- a/packages/block-library/src/calendar/style.scss
+++ b/packages/block-library/src/calendar/style.scss
@@ -11,21 +11,16 @@
font-weight: 400;
}
+ caption {
+ background-color: inherit;
+ }
+
table {
width: 100%;
border-collapse: collapse;
- // Keep the hard-coded header background color for backward compatibility.
- &:where(:not(.has-background)) th {
- background: $gray-300;
- }
-
&:where(:not(.has-text-color)) {
- // Keep the hard-coded body and caption color for backward compatibility.
- tbody,
- caption {
- color: #40464d;
- }
+ color: #40464d;
// Keep the hard-coded border color for backward compatibility.
th,
@@ -33,5 +28,18 @@
border-color: $gray-300;
}
}
+
+ &.has-background th {
+ background-color: inherit;
+ }
+
+ &.has-text-color th {
+ color: inherit;
+ }
}
}
+
+// Keep the hard-coded header background color for backward compatibility.
+:where(.wp-block-calendar table:not(.has-background) th) {
+ background: $gray-300;
+}
- I removed the recently added
tdandcaptionfrom the feature level selectors - Added a style for the
captionso it would inherit any background applied to the table - Simplified the default color fallback. Although it lowers the specificity, I believe this was the intent by introducing the
:whereselector - Restored the previously suggested CSS rules to inherit background color and text color when set at the individual block level
- Moved the default
thbackground color so it was contained within a single:whereselector allowing global styles to override it
The styles might still be able to be cleaned up further but I've run out of time tonight.
Below are demo videos from before my latest suggested changes and after.
| Before (Current PR) | After (With changes Above) |
|---|---|
Screen.Recording.2022-09-15.at.7.08.16.pm.mp4 |
Screen.Recording.2022-09-15.at.7.01.14.pm.mp4 |
|
Thanks for the further suggestions, @aaronrobertshaw ! I was just wrestling with the idea of somehow maintaining all backward compatibility. |
|
I have tested your suggestion, but could not solve the only problem regarding the border color. When I change the text color, the border color should be the same color. But when I change the text color from the global style, it retains the backward compatible $gray-300. 8213cd3f2db0ec15d49147219500d69e.mp4I have done a lot of testing and I think there are two possible choices:
|
There was a problem hiding this comment.
When I change the text color, the border color should be the same color. But when I change the text color from the global style, it retains the backward compatible $gray-300.
This is understandable and a tricky one to solve given when styles are generated for global styles we have nothing to be able to hook onto to override or omit that default border color.
To be clear for others coming to this PR in future:
- Setting text color for individual blocks does result in the default border color being omitted and thus picking up the
currentColorvalue. - It is only the global styles or theme.json application of a text color that does not result in this behaviour.
The matching of the border color to the text color is a new feature. As such I don't think it should trump breaking existing uses of this block out in the wild. Especially, when there is a workaround i.e. setting it on the individual block, even if tedious.
There's also another option. You could opt into border supports for this block. It would provide a means to set the border color at the Calendar block level in the Global Styles or theme.json.
I don't think that needs to block this PR but is definitely something you could do in a follow-up to this.
Given all of that, I believe this PR is in a reasonable place to land. Thank you for your patience and continued iteration towards this end.
Nice work @t-hamano 🚢
✅ Theme.json - Colors applied via theme.json blocks config appear correctly
✅ Global Styles - Global Styles correctly override the theme.json values
✅ Individual Block Styles - Color values set at the individual block level take precedence above others
✅ The caption receives any background color that's been set
✅ The table header receives background color selection
✅ Without a background the table header gets a default grey background
✅ Original hardcoded default values for text, header background, and border colors are maintained.
I see, let's go with this! |
t-hamano
removed
Needs Testing
Fix: #40989
What?
This PR adds color block support to the calendar block and updates the style to match a variety of themes.
Why?
This block has hard-coded text color and border color.
This causes problems that the text is difficult to read and the design doesn't match the other blocks, depending on the background color of the content.
How?
I've made the following changes based on a table block (using the same
tabletag):theadbottom border thickerTesting Instructions
Screenshots or screencast
175876129-b9d22262-cfdd-4499-ba0d-9d9eca0ac6e1.mp4