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

Jump to bottom

Quote v2: update how attributes are registered #39729

Merged
merged 1 commit into from
Mar 25, 2022
Merged

Quote v2: update how attributes are registered #39729

Show file tree
Hide file tree
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
3 changes: 2 additions & 1 deletion packages/block-library/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
"src/**/*.scss",
"src/navigation-link/index.js",
"src/template-part/index.js",
"src/query/index.js"
"src/query/index.js",
"src/quote/v2/index.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious about this change, how do we decide to add a file here.

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding is that we use this to signal webpack not to remove things that aren't part of the export in those files. In theory, it should remove the addFilter call. In practice, though, I tested without adding this and things worked as expected. Not sure if there's some webpack config missing, but wanted to play it safe and avoid the situation in which the v2 block stops working because of some webpack config changed.

],
"dependencies": {
"@babel/runtime": "^7.16.0",
Expand Down
19 changes: 0 additions & 19 deletions packages/block-library/src/quote/v2/block.json
View file
Open in desktop

This file was deleted.

49 changes: 44 additions & 5 deletions packages/block-library/src/quote/v2/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,14 @@
*/
import { __ } from '@wordpress/i18n';
import { quote as icon } from '@wordpress/icons';
import { addFilter } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import metadata from './block.json';
import edit from './edit';
import save from './save';

const { name } = metadata;

export { metadata, name };

const settings = {
icon,
example: {
Expand All @@ -35,3 +31,46 @@ const settings = {
};

export default settings;

/**
* This function updates the attributes for the quote v2.
* This should be moved to block.json when v2 becomes the default.
*
* @param {Array} blockSettings The settings of the block to be registered.
* @param {string} blockName The name of the block to be registered.
* @return {Array} New settings.
*/
function registerQuoteV2Attributes( blockSettings, blockName ) {
if ( ! window?.__experimentalEnableQuoteBlockV2 ) {
return blockSettings;
}

if ( blockName !== 'core/quote' ) {
return blockSettings;
}

// Register the new attribute.
Object.assign( blockSettings.attributes, {
attribution: {
type: 'string',
source: 'html',
selector: 'figcaption',
default: '',
__experimentalRole: 'content',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the issue if we use the same block.json file for both versions. I mean aside having all the old attributes in the version, which doesn't bother me much specially since we need to keep them to implement the "migration effect" behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was the other way around I was concerned about: I didn't want to pollute the existing quote block with the v2 attributes.

Technically, it might work in this case, as we're not modifying existing atts but adding new ones. I can't think of anything that would break. Though I still not a fan of this approach, I rather keep things separated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's keep them separated for now, it doesn't hurt either.

} );

// Deregister the old ones.
delete blockSettings.attributes.value;
delete blockSettings.attributes.citation;

return blockSettings;
}

// Importing this file includes side effects.
// This has been declared in block-library/package.json under sideEffects.
addFilter(
'blocks.registerBlockType',
'core/quote/v2-attributes',
registerQuoteV2Attributes
);