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

Editor: Consider single unmodified default block as empty content #9808

Merged
merged 2 commits into from Sep 17, 2018

Conversation

Projects
None yet
4 participants
@aduth
Member

aduth commented Sep 11, 2018

Related: #9626

This pull request seeks to consider a post which consists only of a single unmodified default block (paragraph) to be empty, rather than serialize as currently:

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

This will be used toward #9626 in preventing a user from deleting all of blocks to an entirely empty post while ensuring that empty post is saved as truly empty.

Implementation notes:

Since the behavior here is similar in form to a previously-added removep logic (#8077), it refactors the approach to consider block saveability by filter extension. This also helps support the need here for preserving existing logic of "is saveable", where the omission of the unmodified default block can be a factor in consideration. It introduces two new filterable selectors getBlocksForSave and getBlockContent.

Testing instructions:

Verify that a post is not saveable once clicking into the writing prompt.

Verify that a post's content is correctly saved once entered.

Verify that if a title is assigned but content exists only as the unmodified default block, that the saved content is empty.

Verify that removep is applied to the unknown block type (freeform).

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 12, 2018

Member

Sitting on it for the night, I'm open to a challenge that these selectors don't need to be filterable vs. having the hook behaviors built-in to the selectors. There were a few various approaches I'd taken to addressing this general need for "save-time processing" which ultimately landed me here. It also touched on some thoughts around #7020 where we could allow the results of validity-related selectors to be extended by plugin authors.

Member

aduth commented Sep 12, 2018

Sitting on it for the night, I'm open to a challenge that these selectors don't need to be filterable vs. having the hook behaviors built-in to the selectors. There were a few various approaches I'd taken to addressing this general need for "save-time processing" which ultimately landed me here. It also touched on some thoughts around #7020 where we could allow the results of validity-related selectors to be extended by plugin authors.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Sep 12, 2018

Member

I'm open to a challenge that these selectors don't need to be filterable vs. having the hook behaviors built-in to the selectors.

I'm leaning more towards removing the filters and having the logic directly inline. These new filters are very low-level. I think history has show that low-level filters become very rigid and can make it easier for third-party code to cause unexpected behaviors. It's not clear there's an immediate need for this abstraction.

It also touched on some thoughts around #7020 where we could allow the results of validity-related selectors to be extended by plugin authors.

Is this pull request the best way to address #7020, or is it better to hold off until we design an API specifically for it?

Member

danielbachhuber commented Sep 12, 2018

I'm open to a challenge that these selectors don't need to be filterable vs. having the hook behaviors built-in to the selectors.

I'm leaning more towards removing the filters and having the logic directly inline. These new filters are very low-level. I think history has show that low-level filters become very rigid and can make it easier for third-party code to cause unexpected behaviors. It's not clear there's an immediate need for this abstraction.

It also touched on some thoughts around #7020 where we could allow the results of validity-related selectors to be extended by plugin authors.

Is this pull request the best way to address #7020, or is it better to hold off until we design an API specifically for it?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 12, 2018

Member

Is this pull request the best way to address #7020, or is it better to hold off until we design an API specifically for it?

Nothing here directly addresses #7020. Mentioned more in terms of framing my mindset as I'd taken a few different approaches before landing on this one. It's only upon taking a step back that I observe that it may have been prematurely optimized.

Member

aduth commented Sep 12, 2018

Is this pull request the best way to address #7020, or is it better to hold off until we design an API specifically for it?

Nothing here directly addresses #7020. Mentioned more in terms of framing my mindset as I'd taken a few different approaches before landing on this one. It's only upon taking a step back that I observe that it may have been prematurely optimized.

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Sep 13, 2018

Member

I agree with @danielbachhuber's comment, low level filters are generally a problem for long term maintenance.

I'd be inclined to do it inline for now, we can always consider adding filters if there's a demand for them. Filters are always better added when there's a practical use case to go with them.

Member

pento commented Sep 13, 2018

I agree with @danielbachhuber's comment, low level filters are generally a problem for long term maintenance.

I'd be inclined to do it inline for now, we can always consider adding filters if there's a demand for them. Filters are always better added when there's a practical use case to go with them.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 13, 2018

Member

Semi-blocking comment at #5185 questioning whether an intent here would be "the sole paragraph block" vs. "trailing paragraph block" vs. "trailing paragraph blocks" vs. "leading or trailing paragraph blocks" vs. "any empty paragraph block".

Member

aduth commented Sep 13, 2018

Semi-blocking comment at #5185 questioning whether an intent here would be "the sole paragraph block" vs. "trailing paragraph block" vs. "trailing paragraph blocks" vs. "leading or trailing paragraph blocks" vs. "any empty paragraph block".

@aduth aduth added this to the 4.0 milestone Sep 14, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 14, 2018

Member

In an effort to keep this moving along, I'm going to stick with the most conservative and original proposal of this pull request: removing only in the case of the unmodified default block being the sole block in the editor.

Rebased and moved post-processing back inline to the selectors.

Member

aduth commented Sep 14, 2018

In an effort to keep this moving along, I'm going to stick with the most conservative and original proposal of this pull request: removing only in the case of the unmodified default block being the sole block in the editor.

Rebased and moved post-processing back inline to the selectors.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 17, 2018

Contributor

I noticed a small bug when testing this PR, I get an "Updating failed" error when auto-saving an empty post. I'm not certain about the exact steps to reproduce and I'm also not certain about whether this is specific to this PR

Contributor

youknowriad commented Sep 17, 2018

I noticed a small bug when testing this PR, I get an "Updating failed" error when auto-saving an empty post. I'm not certain about the exact steps to reproduce and I'm also not certain about whether this is specific to this PR

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 17, 2018

Member

I get an "Updating failed" error when auto-saving an empty post.

To clarify, by "empty post", do you mean one which has a title but no content? Or neither title nor content (nor excerpt)?

Member

aduth commented Sep 17, 2018

I get an "Updating failed" error when auto-saving an empty post.

To clarify, by "empty post", do you mean one which has a title but no content? Or neither title nor content (nor excerpt)?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 17, 2018

Member

I've tried various combinations of empty title / content to try to find a failing autosave case, but haven't had any luck. @youknowriad can you elaborate on the steps necessary to reproduce?

In the rebased 4b4b014 I've updated an end-to-end test-case which would have had its snapshot content (expectedly) changed. I opted to change the test itself to make it more clear at resolving the original regression.

Member

aduth commented Sep 17, 2018

I've tried various combinations of empty title / content to try to find a failing autosave case, but haven't had any luck. @youknowriad can you elaborate on the steps necessary to reproduce?

In the rebased 4b4b014 I've updated an end-to-end test-case which would have had its snapshot content (expectedly) changed. I opted to change the test itself to make it more clear at resolving the original regression.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 17, 2018

Contributor

I forgot how exactly it happened because I was testing various changes. The thing is I'm not able to reproduce anymore.

Contributor

youknowriad commented Sep 17, 2018

I forgot how exactly it happened because I was testing various changes. The thing is I'm not able to reproduce anymore.

@youknowriad

LGTM 👍

@aduth aduth merged commit dff35fb into master Sep 17, 2018

2 checks passed

codecov/project 48.77% (+0.04%) compared to 0816e23
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the update/empty-single-unmodified-default-block branch Sep 17, 2018

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Sep 29, 2018

Member

I noticed a small bug when testing this PR, I get an "Updating failed" error when auto-saving an empty post. I'm not certain about the exact steps to reproduce and I'm also not certain about whether this is specific to this PR

@youknowriad I was able to reproduce this issue. The problem is the same as #6556. Steps to reproduce are:

  1. Add a new post.
  2. Insert a Classic block
  3. Wait a while
Member

danielbachhuber commented Sep 29, 2018

I noticed a small bug when testing this PR, I get an "Updating failed" error when auto-saving an empty post. I'm not certain about the exact steps to reproduce and I'm also not certain about whether this is specific to this PR

@youknowriad I was able to reproduce this issue. The problem is the same as #6556. Steps to reproduce are:

  1. Add a new post.
  2. Insert a Classic block
  3. Wait a while
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment