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: Prevent attribute styles/classes from printing twice #43653

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Calendar: Prevent attribute styles/classes from printing twice #43653

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 2 deletions packages/block-library/src/calendar/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const getYearMonth = memoize( ( date ) => {
};
} );

export default function CalendarEdit( { attributes } ) {
export default function CalendarEdit() {
const blockProps = useBlockProps();
const { date, hasPosts, hasPostsResolved } = useSelect( ( select ) => {
const { getEntityRecords, hasFinishedResolution } = select( coreStore );
Expand Down Expand Up @@ -95,7 +95,7 @@ export default function CalendarEdit( { attributes } ) {
<Disabled>
<ServerSideRender
block="core/calendar"
attributes={ { ...attributes, ...getYearMonth( date ) } }
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 that if we remove the attributes prop from the ServerSideRender component, we would not be able to dynamically generate styles and markup based on attirbutes, including block support such as Dimension.
For example, #42029 uses attributes to apply a color to a table tag inside a calendar block.

I have addressed my concerns and suggestions in my comments here, but I think we should have a little more discussion about the policy for addressing the addition of support for blocks using the ServiersideRender component.

Also, as mentioned within the comments above, there are five blocks that use the ServersideRender component, and duplicate class names might need to be resolved in all of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good questions, thanks for opening the PR @ndiego and for the ideas here @t-hamano!

I was wondering if another option rather than preventing passing attributes to the ServerSideRender component would be to strip the style key from the rest expansion of ...useBlockProps() on the wrapper div? That way we might be able to defer to the ServerSideRender for all block style output when we're using the server side rendering in the editor.

I haven't tried it, and there could be edge cases with that idea, but just thought I'd raise it in case it seems like another viable option.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a semi-related PR that selectively removed the test-decoration for the post comments count block's editor rendering (#43497) — while that one was to do with preventing style output that affects the placeholder state, we might be able to use a similar approach in this (and other blocks that use ServerSideRender) to skip the style output at the wrapper div level 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@ndiego
@andrewserong

I raised the new approach in my comment here in reference to #43497. I would love to hear your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the discussion that took place in comment #43243, I think the following might be a good approach.

  • Don't pass margin and padding with block support to ServersideRender component (it is simpler to remove spacing all together than to limit it to margin alone, which simplifies the code)

  • As for className, only give it to the block wrapper div, as per this PR policy

Here is an example code:

//  Attributes passed to ServerSideRender components.
const serverSideRenderAttirbutes = {
	...attributes,
	...getYearMonth( date ),
	// Exclude additional CSS class(es).
	className: undefined,
	// Exclude spacing block support.
	style: {
		...attributes.style,
		spacing: undefined,
	},
};

return (
	<div { ...blockProps }>
		<Disabled>
			<ServerSideRender
				block="core/calendar"
				attributes={ serverSideRenderAttirbutes }
			/>
		</Disabled>
	</div>
);

I think we shuldn't use omit, etc, because I found that #17025 is trying to move away from Lodash.
This code also takes into account the spacing support that will be added in #43654. However, we will need to brush up on this code for color support, which will be added in #42029.

@ndiego
@andrewserong
We are looking forward to your advice.

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 for the feedback @t-hamano, sorry for the delay, been tied up with WCUS prep. I will take a look at this approach this weekend!

Copy link
Member

Choose a reason for hiding this comment

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

Just closing the loop on the above comment for the omit usage - #17025 has landed and we're no longer using Lodash in the entire repository.

attributes={ { ...getYearMonth( date ) } }
/>
</Disabled>
</div>
Expand Down