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

fix/2854: getSourcedAttributes was not completely removed #3582

Merged
merged 2 commits into from
Nov 21, 2017

Conversation

BoardJames
Copy link

@BoardJames BoardJames commented Nov 21, 2017

It was still being used leading to a runtime error in block validation.

Description

The PR #2854 removed getSourcedAttributes but it was still exported in the api/index.js and it was still used in validation of block content after editing as HTML.

Note that this may fix issue #3423 as with this fix I can't replicate a block crash only a "This block has been changed externally" dialog. I wonder though why we don't give an option of continuing to edit the HTML (not converting to a HTML block) from that dialog.

How Has This Been Tested?

Manual tests in Firefox and Chrome.

Types of changes

Bug fix.

Checklist:

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

It was still being used leading to a runtime error in block validation.
@BoardJames BoardJames added the [Type] Bug An existing feature does not function as intended label Nov 21, 2017
@BoardJames BoardJames self-assigned this Nov 21, 2017
@codecov
Copy link

codecov bot commented Nov 21, 2017

Codecov Report

Merging #3582 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3582      +/-   ##
==========================================
+ Coverage   36.92%   36.92%   +<.01%     
==========================================
  Files         267      267              
  Lines        6663     6662       -1     
  Branches     1203     1203              
==========================================
  Hits         2460     2460              
+ Misses       3551     3550       -1     
  Partials      652      652
Impacted Files Coverage Δ
editor/components/block-list/block-html.js 0% <0%> (ø) ⬆️

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 981c22a...04f5fcb. Read the comment docs.

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.

Good catch. Thanks for the fix. I think adding a unit test or end2end test for the HTML mode would be good to avoid future regressions.

@youknowriad
Copy link
Contributor

I wonder though why we don't give an option of continuing to edit the HTML (not converting to a HTML block) from that dialog.

Yes agreed, we could show the dialog only when returning to the visual mode but I believe this requires some refactoring to achieve.

@BoardJames BoardJames merged commit 5a9bf8a into master Nov 21, 2017
@BoardJames BoardJames deleted the fix/2854-removed-export-of-getSourcedAttributes branch November 21, 2017 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants