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.

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

Parser: Parse superfluous classes as custom classes #7538

Merged
merged 2 commits into from
Jun 28, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 25, 2018

Closes #5028
Maybe addresses #6826 (?)
Related: aduth/hpq#2

This pull request seeks to enable a block which supports customClassName (default true) to inherit as part of its className attribute any classes which the user has added manually. This avoids a block being marked as having been modified externally, and further treats the extra classes the same as any other custom class (specifically in exposing by its Inspector > Advanced > Additional CSS Class).

Implementation notes:

This adds a new filter on the parsed block attributes, determining if there is a difference between how classes were parsed and what was received as original markup of the block.

Noting that there is some relation between this and aduth/hpq#2 , particularly in how we retrieve the class attribute from the root element of the markup. Currently this requires adding a dummy wrapper node. The changes proposed in aduth/hpq#2 may still be in our interest to explore, but would be a significant breaking change affecting a large number of existing blocks. At this point, we may just want to be content with the current behavior, despite its drawbacks.

Testing instructions:

Verify that adding a class to a block which supports custom class names (e.g. paragraph) via the block menu's Edit as HTML view does not invalidate the block, is persisted between editor sessions, and is displayed in its Additional CSS Class advanced inspector field.

@aduth aduth added [Feature] Blocks Overall functionality of blocks [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f labels Jun 25, 2018
@youknowriad
Copy link
Contributor

Correct if I'm wrong but we're still persisting the custom className in the comment of blocks. I wonder if it's possible to drop it entirely from there with this new filter. I'm thinking of this extra filter as a way to define custom "source" in the block parser.

@aduth
Copy link
Member Author

aduth commented Jun 26, 2018

Correct if I'm wrong but we're still persisting the custom className in the comment of blocks. I wonder if it's possible to drop it entirely from there with this new filter. I'm thinking of this extra filter as a way to define custom "source" in the block parser.

It may be possible, yes, though I'm actually motivated to not allow for custom sources, particularly with how it impacts the viability of ever being able to parse blocks from the server. The current set of supported sources could be recreated on the server; if we allow for custom sources, not so much.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I think I'd prefered to drop the className comment attribute entirely but I'm fine with this too.

@@ -148,7 +149,13 @@ export function getBlockAttributes( blockType, innerHTML, attributes ) {
return getBlockAttribute( attributeKey, attributeSchema, innerHTML, attributes );
} );

return blockAttributes;
return applyFilters(
'blocks.getBlockAttributes',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to document this or keep it "secret" for now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to document this or keep it "secret" for now :)

I'm fine to document. Will add.

if ( filteredClassName ) {
blockAttributes.className = filteredClassName;
} else {
delete blockAttributes.className;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not certain why we're deleting the className attribute in this case? This creates this bug #9991. Can you clarify a bit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh actually the filsterClassName variable have been removed at some point which is causing the bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aduth aduth added the [Feature] Block Validation/Deprecation Handling block validation to determine accuracy and deprecation label Jan 6, 2020
@Buddierdl
Copy link

Hi, this seems to have been broken in the upgrade from WP 5.8 to 5.9. Now custom classes on HTML content are being dropped again when converting to blocks using the rawHandler. Anyone else seeing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Validation/Deprecation Handling block validation to determine accuracy and deprecation [Feature] Blocks Overall functionality of blocks [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persist custom CSS classes during block conversion when block supports additional classes
3 participants