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

Add inlineStyle matcher for sourcing block attributes #10074

Closed
wants to merge 1 commit into from

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Sep 20, 2018

Allows sourcing of a block attribute from an inline style

Description

I added this as part of #9801, but thought I'd break it out into a separate PR so that it can undergo a bit more scrutiny (and to make that PR a bit smaller).

#9801 adds width and height controls for tables in a table block and I wanted to be able to source the height and width directly from the inline styles. When pasting markup, the attributes can then be sourced directly from the styles instead of the json object, as demonstrated by this gif:
pasting-table

I think its also advantageous to encode attributes in the markup when possible for portability. It might also be possible to use this matcher in other existing blocks (e.g. image) that also have width and height attributes that result in inline styles.

This is the first time I've touched matchers, so it'd be good to know if there are any downsides to this approach.

How has this been tested?

I tested across all supported desktop browsers and this seems to work everywhere. MS Edge has a separate bug that I discovered in testing and there's a fix for that here:
#10077

I did manage to verify that this works when the edge fix was applied to this branch.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Allows sourcing of a block attribute from an inline style
@talldan talldan added [Type] Enhancement A suggestion for improvement. [Feature] Block API API that allows to express the block paradigm. labels Sep 20, 2018
@talldan talldan self-assigned this Sep 20, 2018
@talldan talldan requested a review from aduth September 20, 2018 15:50
@aduth
Copy link
Member

aduth commented Sep 21, 2018

This is the first time I've touched matchers, so it'd be good to know if there are any downsides to this approach.

I think we should be very cautious about adding new matchers, for a few reasons:

  • There's some desire that we could support our set of matchers in server-side parsing (Block API: Server-side awareness of block types #2751). The difficulty as it exists today is in having a reliable / performant HTML parse. Anything we add as a matcher which moves us further from this should be scrutinized. In this case, supporting inline styles would require a CSS parse implementation to recreate on the server. This is also part of why the underlying property matcher was removed in Block API: Deprecate property source #7349, since recreating a DOM object server-side would be near-impossible.
  • Limiting the set of matchers that a block developer would be expected to learn, particularly in cases where alternatives may exist. Having multiple options to achieve the same goal does not seem like a necessarily good thing.
  • Encouraging best practices by making the bad practices less accessible. I'm not sure whether it applies here, but in most cases we'd probably want to discourage use of inline styles in favor of classes which are (a) arguably more semantically understandable, (b) open to future revisions without invalidations, and (c) easier to extend than style values.

In this case, is it something we could have tracked as a comment attribute, even if that's then duplicated from there and the markup? Given above, I'd be more okay with the smaller trade-off of some redundancy in the comment attributes.

@talldan
Copy link
Contributor Author

talldan commented Sep 21, 2018

@aduth thanks for the detailed feedback. Given your points, it does seem sensible to use comment attributes - I'll switch to that in my PR for the table block improvements and close this.

@talldan talldan closed this Sep 21, 2018
@talldan talldan deleted the add/inline-style-attribute-matcher branch September 21, 2018 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants