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

Validation: Log debugging messages for invalid blocks #3522

Merged
merged 1 commit into from Nov 20, 2017

Conversation

Projects
None yet
4 participants
@aduth
Member

aduth commented Nov 16, 2017

Closes #2837

This pull request seeks to add logging to the block validator to emit messages when an invalid block is encountered in a development environment.

Validation

Testing instructions:

Modify post-content.js, removing, adding, or changing attributes or content which would cause a block to become invalid (i.e. serialize differently on the next load).

Load the editor and verify that debugging messages are logged in your Developer Tools console.

@aduth aduth added the Framework label Nov 16, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 16, 2017

Codecov Report

Merging #3522 into master will increase coverage by 0.13%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3522      +/-   ##
==========================================
+ Coverage   35.15%   35.29%   +0.13%     
==========================================
  Files         267      267              
  Lines        6724     6744      +20     
  Branches     1218     1221       +3     
==========================================
+ Hits         2364     2380      +16     
- Misses       3683     3687       +4     
  Partials      677      677
Impacted Files Coverage Δ
blocks/api/validation.js 91.46% <88.23%> (-3.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d50d76...cd212c1. Read the comment docs.

codecov bot commented Nov 16, 2017

Codecov Report

Merging #3522 into master will increase coverage by 0.13%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3522      +/-   ##
==========================================
+ Coverage   35.15%   35.29%   +0.13%     
==========================================
  Files         267      267              
  Lines        6724     6744      +20     
  Branches     1218     1221       +3     
==========================================
+ Hits         2364     2380      +16     
- Misses       3683     3687       +4     
  Partials      677      677
Impacted Files Coverage Δ
blocks/api/validation.js 91.46% <88.23%> (-3.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d50d76...cd212c1. Read the comment docs.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 20, 2017

Member

I enabled logging for all environs which, with global test console assertions, helped identify many legitimate issues both in parser testing, and in the block fixtures. Those have been fixed here. All-environment logging should be helpful for plugin usage to report issues to the original block implementer.

Member

aduth commented Nov 20, 2017

I enabled logging for all environs which, with global test console assertions, helped identify many legitimate issues both in parser testing, and in the block fixtures. Those have been fixed here. All-environment logging should be helpful for plugin usage to report issues to the original block implementer.

@aduth aduth merged commit 3b4e8ef into master Nov 20, 2017

3 checks passed

codecov/project 35.29% (+0.13%) compared to 2d50d76
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the add/validation-info branch Nov 20, 2017

@ahmadawais

This comment has been minimized.

Show comment
Hide comment
@ahmadawais

ahmadawais Dec 10, 2017

Contributor

@aduth where can I read more about the validation API?
I figure it must be this file https://github.com/WordPress/gutenberg/blob/db507cd/blocks/api/validation.js

One of the demo tweet blocks I had built it now renders corrupted.
image

With this log message

image

Whereas I have myself changed the href in save: part of wp.blocks.registerBlockType API.
E.g.

Code present here.

Looking forward!

Contributor

ahmadawais commented Dec 10, 2017

@aduth where can I read more about the validation API?
I figure it must be this file https://github.com/WordPress/gutenberg/blob/db507cd/blocks/api/validation.js

One of the demo tweet blocks I had built it now renders corrupted.
image

With this log message

image

Whereas I have myself changed the href in save: part of wp.blocks.registerBlockType API.
E.g.

Code present here.

Looking forward!

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 11, 2017

Member

@ahmadawais Validation works by comparing what would be saved when running save against what is actually saved in post content, considering whether the two markups are "equivalent". In your example, it's finding that the href value is not as expected. Based on the fact that href is assigned using the tweet content, the content attribute is not being parsed correctly when loading from an existing post.

Looking at the code example, I see you're expecting to find a children source in a paragraph tag. But you're not saving a paragraph tag from your block; you're saving an a (anchor) tag. Changing the selector might be enough, but I also think you could run into issues as children value can't easily be passed to encodeURIComponent since it's not a string. In fact, it is a React element, so you could pass it to wp.element.renderToString, but ideally not since the fact that it's represented as a React element is an implementation detail and is subject to change (see also #3048).

This is an interesting use-case, since you want access to a string representation of the rich text content to be able to encode in a tweet URL. I might imagine depending on the direction we go with #3048 that we'd want some way to convert from a children value to a string (cc @iseulde, @gziolo).

Member

aduth commented Dec 11, 2017

@ahmadawais Validation works by comparing what would be saved when running save against what is actually saved in post content, considering whether the two markups are "equivalent". In your example, it's finding that the href value is not as expected. Based on the fact that href is assigned using the tweet content, the content attribute is not being parsed correctly when loading from an existing post.

Looking at the code example, I see you're expecting to find a children source in a paragraph tag. But you're not saving a paragraph tag from your block; you're saving an a (anchor) tag. Changing the selector might be enough, but I also think you could run into issues as children value can't easily be passed to encodeURIComponent since it's not a string. In fact, it is a React element, so you could pass it to wp.element.renderToString, but ideally not since the fact that it's represented as a React element is an implementation detail and is subject to change (see also #3048).

This is an interesting use-case, since you want access to a string representation of the rich text content to be able to encode in a tweet URL. I might imagine depending on the direction we go with #3048 that we'd want some way to convert from a children value to a string (cc @iseulde, @gziolo).

@ahmadawais

This comment has been minimized.

Show comment
Hide comment
@ahmadawais

ahmadawais Jan 1, 2018

Contributor

@aduth just returning back from vacations and was out of all the Gutenberg dev but want to know if there has been any development in this particular aspect? Any pointers that you have for me to go and read about. As I am sure there's a possibility of having different edit and save interfaces since the saved interface should be clickable whereas edit interface should not.

Looking forward!

Contributor

ahmadawais commented Jan 1, 2018

@aduth just returning back from vacations and was out of all the Gutenberg dev but want to know if there has been any development in this particular aspect? Any pointers that you have for me to go and read about. As I am sure there's a possibility of having different edit and save interfaces since the saved interface should be clickable whereas edit interface should not.

Looking forward!

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jan 2, 2018

Member

@ahmadawais Is there any reason the Tweetable text needs to be "rich text" in your case (i.e. supporting italics, bold, etc)? Thinking it might make more sense as plain text, especially since you can't tweet the rich text anyways. In which case you could use a simple text input instead of Editable. The value could either be saved in the comment attributes, or sourced / converted from the saved anchor value. Thinking something like:

content: {
	type: 'string',
	source: 'text',
	selector: 'a',
},
Member

aduth commented Jan 2, 2018

@ahmadawais Is there any reason the Tweetable text needs to be "rich text" in your case (i.e. supporting italics, bold, etc)? Thinking it might make more sense as plain text, especially since you can't tweet the rich text anyways. In which case you could use a simple text input instead of Editable. The value could either be saved in the comment attributes, or sourced / converted from the saved anchor value. Thinking something like:

content: {
	type: 'string',
	source: 'text',
	selector: 'a',
},
@ahmadawais

This comment has been minimized.

Show comment
Hide comment
@ahmadawais

ahmadawais Jan 2, 2018

Contributor

@aduth No not at all. I was actually trying to make it work without any of that, didn't spend a lot of time on it, but didn't get around producing an editable text block that's not rich text. How does one do that?

Also, my guess is that validation won't apply on a case like this with simple text block? I just need the user to write the Tweet's content.

Contributor

ahmadawais commented Jan 2, 2018

@aduth No not at all. I was actually trying to make it work without any of that, didn't spend a lot of time on it, but didn't get around producing an editable text block that's not rich text. How does one do that?

Also, my guess is that validation won't apply on a case like this with simple text block? I just need the user to write the Tweet's content.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jan 2, 2018

Member

but didn't get around producing an editable text block that's not rich text. How does one do that?

Instead of Editable, you could just use a plain input type. Similar to Editable, it should behave as a simple controlled component:

https://reactjs.org/docs/forms.html#controlled-components

Also, my guess is that validation won't apply on a case like this with simple text block?

Not entirely clear what you mean here, but with the plain text input value, it is easy enough to encodeURIComponent so I wouldn't expect a validation error to occur if the regenerated markup is equivalent.

Member

aduth commented Jan 2, 2018

but didn't get around producing an editable text block that's not rich text. How does one do that?

Instead of Editable, you could just use a plain input type. Similar to Editable, it should behave as a simple controlled component:

https://reactjs.org/docs/forms.html#controlled-components

Also, my guess is that validation won't apply on a case like this with simple text block?

Not entirely clear what you mean here, but with the plain text input value, it is easy enough to encodeURIComponent so I wouldn't expect a validation error to occur if the regenerated markup is equivalent.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 11, 2018

Contributor

@aduth @iseulde I've seen this conundrum with people a few times, where they want the display of editable (no borders, consistent font-size, etc) but don't care about rich text. What about adding a mode to Editable via prop to get plain text (would render just a textarea with not tinymce) instead of styling inputs?

Contributor

mtias commented Jan 11, 2018

@aduth @iseulde I've seen this conundrum with people a few times, where they want the display of editable (no borders, consistent font-size, etc) but don't care about rich text. What about adding a mode to Editable via prop to get plain text (would render just a textarea with not tinymce) instead of styling inputs?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jan 11, 2018

Member

@aduth @iseulde I've seen this conundrum with people a few times, where they want the display of editable (no borders, consistent font-size, etc) but don't care about rich text. What about adding a mode to Editable via prop to get plain text (would render just a textarea with not tinymce) instead of styling inputs?

Another benefit to this is that if people are accustomed to working with Editable, they'd be inclined to use it for all text fields, and at least if we provide them the option to configure it for plain text, it would be fine enough to use.

The downside is that value of Editable would be overloaded to support both plain strings and the rich text value format.

Member

aduth commented Jan 11, 2018

@aduth @iseulde I've seen this conundrum with people a few times, where they want the display of editable (no borders, consistent font-size, etc) but don't care about rich text. What about adding a mode to Editable via prop to get plain text (would render just a textarea with not tinymce) instead of styling inputs?

Another benefit to this is that if people are accustomed to working with Editable, they'd be inclined to use it for all text fields, and at least if we provide them the option to configure it for plain text, it would be fine enough to use.

The downside is that value of Editable would be overloaded to support both plain strings and the rich text value format.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Jan 11, 2018

Member

@mtias @aduth I'm a bit hesitant about this as the component attributes are so incredibly different. I think it makes sense to keep them separated, maybe we just need better naming than Editable (sounds general) and TextareaAutosize. How about just RichText and PlainText?

Member

iseulde commented Jan 11, 2018

@mtias @aduth I'm a bit hesitant about this as the component attributes are so incredibly different. I think it makes sense to keep them separated, maybe we just need better naming than Editable (sounds general) and TextareaAutosize. How about just RichText and PlainText?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment