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

Calendar Block: Add color supports and polish styles #42029

Merged
merged 16 commits into from
Sep 20, 2022
Merged

Calendar Block: Add color supports and polish styles #42029

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
78b11c0
Calender Block: Add color supports and polish styles
t-hamano Jun 27, 2022
1e42f5f
Add background-color support, add color and style dynamic rendering, …
t-hamano Aug 21, 2022
a5cfa4a
Fix php lint error
t-hamano Aug 21, 2022
85a4e7f
Merge branch 'trunk' into try/block-calendar
t-hamano Sep 10, 2022
8b4e335
Remove unnecessary border styles
t-hamano Sep 10, 2022
5adb2a6
Lower specificity of background color for backward compatibility
t-hamano Sep 10, 2022
d2f4b11
Enable color support by global style
t-hamano Sep 10, 2022
3fb8847
Use style-engine for inline CSS
t-hamano Sep 10, 2022
1db52cc
USe style-engine for class names
t-hamano Sep 10, 2022
9c9fb21
Style engine covers more, improv backward compatibility
t-hamano Sep 12, 2022
1b9a074
Add more backward compatibility
t-hamano Sep 12, 2022
7cf7a3e
Fix php lint error
t-hamano Sep 12, 2022
db66153
Merge branch 'trunk' into try/block-calendar
t-hamano Sep 12, 2022
0cf2f17
Fix: global style text color isn't applied to td
t-hamano Sep 12, 2022
63f6bcc
Fix: global style text color isn't applied to caption
t-hamano Sep 12, 2022
35cab08
Fix regressions
t-hamano Sep 18, 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
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 @@ -68,7 +68,7 @@ A calendar of your site’s posts. ([Source](https://github.com/WordPress/gutenb

- **Name:** core/calendar
- **Category:** widgets
- **Supports:** align
- **Supports:** align, color (background, link, text)
- **Attributes:** month, year

## Categories List
Expand Down
10 changes: 9 additions & 1 deletion packages/block-library/src/calendar/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,15 @@
}
},
"supports": {
"align": true
"align": true,
"color": {
"link": true,
"__experimentalSkipSerialization": [ "text", "background" ],
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
"__experimentalDefaultControls": {
"background": true,
"text": true
}
}
},
"style": "wp-block-calendar"
}
68 changes: 67 additions & 1 deletion packages/block-library/src/calendar/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,17 @@ function render_block_core_calendar( $attributes ) {
}
}

$inline_styles = styles_for_block_core_calendar( $attributes );
$color_classes = get_color_classes_for_block_core_calendar( $attributes );
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably leverage the style engine to generate all the color class names and inline styles. See lib/block-supports/colors.php for how the block supports do this when serialization isn't skipped.


$calendar = str_replace( '<table', '<table ' . $inline_styles, get_calendar( true, false ) );
$calendar = str_replace( 'class="wp-calendar-table', 'class="wp-calendar-table ' . $color_classes, $calendar );

$wrapper_attributes = get_block_wrapper_attributes();
$output = sprintf(
'<div %1$s>%2$s</div>',
$wrapper_attributes,
get_calendar( true, false )
$calendar
);

// phpcs:ignore WordPress.WP.GlobalVariablesOverride.OverrideProhibited
Expand All @@ -69,6 +75,66 @@ function register_block_core_calendar() {

add_action( 'init', 'register_block_core_calendar' );

/**
* Builds an array of inline styles for the calendar block.
*
* @param array $attributes The block attributes.
*
* @return string Style HTML attribute.
*/
function styles_for_block_core_calendar( $attributes ) {
$table_styles = array();

// Add color styles.
$has_text_color = ! empty( $attributes['style']['color']['text'] );
if ( $has_text_color ) {
$table_styles[] = sprintf( 'color: %s;', esc_attr( $attributes['style']['color']['text'] ) );
}

$has_background_color = ! empty( $attributes['style']['color']['background'] );
if ( $has_background_color ) {
$table_styles[] = sprintf( 'background-color: %s;', esc_attr( $attributes['style']['color']['background'] ) );
}

return ! empty( $table_styles ) ? sprintf( 'style="%s"', safecss_filter_attr( implode( ' ', $table_styles ) ) ) : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

If we continue to manually generate the styles here free of the style engine, perhaps we can move the esc_attr calls here to wrap safecss_filter_attr to help ensure any future styles added to this function don't miss proper escaping.

}

/**
* Returns color classnames depending on whether there are named or custom text and background colors.
*
* @param array $attributes The block attributes.
*
* @return string The color classnames to be applied to the block elements.
*/
function get_color_classes_for_block_core_calendar( $attributes ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might be able to reduce some of the duplicate logic in class generation if we leverage the style engine.

$classnames = array();

// Text color.
$has_named_text_color = ! empty( $attributes['textColor'] );
$has_custom_text_color = ! empty( $attributes['style']['color']['text'] );
if ( $has_named_text_color ) {
$classnames[] = sprintf( 'has-text-color has-%s-color', $attributes['textColor'] );
} elseif ( $has_custom_text_color ) {
// If a custom 'textColor' was selected instead of a preset, still add the generic `has-text-color` class.
$classnames[] = 'has-text-color';
}

// Background color.
$has_named_background_color = ! empty( $attributes['backgroundColor'] );
$has_custom_background_color = ! empty( $attributes['style']['color']['background'] );
if (
$has_named_background_color ||
$has_custom_background_color
) {
$classnames[] = 'has-background';
}
if ( $has_named_background_color ) {
$classnames[] = sprintf( 'has-%s-background-color', $attributes['backgroundColor'] );
}

return implode( ' ', $classnames );
}

/**
* Returns whether or not there are any published posts.
*
Expand Down
25 changes: 8 additions & 17 deletions packages/block-library/src/calendar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,31 +2,22 @@
text-align: center;

th,
tbody td {
td {
padding: 0.25em;
border: 1px solid $gray-300;
}

tfoot td {
border: none;
border: 1px solid;
}

table {
width: 100%;
border-collapse: collapse;
}

table th {
font-weight: 400;
background: $gray-300;
}
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved

a {
text-decoration: underline;
// Keep the hard-coded header background color for backward compatibility.
&:not(.has-text-color):not(.has-background) th {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think increasing the specifity here is a great option to apply a default. Maybe we could leverage :where instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by 5adb2a6

background: $gray-300;
}
}

table tbody,
table caption {
color: #40464d;
thead {
border-bottom: 3px solid;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry it looks like my comment didn't send the first time:

What's the reason for adding this? I think we should be leaning towards removing styles rather than adding them

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 would like you to watch this video regarding the style comparison with table blocks in my previous comment, The table block has a similar border under thead.
I have made the same changes to the calendar block for style consistency, but do you think this is unnecessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that this will change the appearance of the calendar blocks that people are already using, so I'd rather err on the side of caution and not add more styles.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to not introducing any new styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'm going to remove this style 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by 8b4e335

}
}