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 image insert console error #3634

Merged

Conversation

vladanost
Copy link
Contributor

Description

Fixes #3633

Editable component is complaining about the caption value type.
On new Image its an empty string and Editable expects an array (caption is registered as type array) or undefined so I set it to undefined if empty string.

I tried setting a default value when registering block type to empty array but that does not work.

caption: {
    type: 'array',
    source: 'children',
    selector: 'figcaption',
    default: [],
},

I would also assume that since caption is registered as type array that default value would be empty array. But I guess it doesn't work that way.

How Has This Been Tested?

Manually tested in the browser.
Current js unit tests pass.

@youknowriad
Copy link
Contributor

Thanks for catching this bug,

I investigated a bit and I think the issue we have is elsewhere. When we select an image from the media library, we copy it's caption and it's always a string, we should not copy the caption if it's empty and we should wrap it in an array if it's not empty.

See the onSelectImage function in the image block.

@vladanost
Copy link
Contributor Author

@youknowriad Thanks. Your suggestion seems to work.

@codecov
Copy link

codecov bot commented Nov 24, 2017

Codecov Report

Merging #3634 into master will decrease coverage by 0.05%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3634      +/-   ##
==========================================
- Coverage   36.95%   36.89%   -0.06%     
==========================================
  Files         268      275       +7     
  Lines        6673     6632      -41     
  Branches     1202     1202              
==========================================
- Hits         2466     2447      -19     
+ Misses       3555     3532      -23     
- Partials      652      653       +1
Impacted Files Coverage Δ
blocks/library/image/block.js 1.72% <0%> (-0.1%) ⬇️
editor/reducer.js 90% <0%> (-0.28%) ⬇️
editor/selectors.js 92.85% <0%> (-0.27%) ⬇️
.../edit-post/sidebar/document-outline-panel/index.js 0% <0%> (ø) ⬆️
editor/edit-post/modes/visual-editor/index.js 0% <0%> (ø) ⬆️
editor/edit-post/layout/index.js 0% <0%> (ø) ⬆️
editor/edit-post/sidebar/post-trash/index.js 0% <0%> (ø) ⬆️
editor/edit-post/header/header-toolbar/index.js 0% <0%> (ø) ⬆️
editor/components/block-list/index.js 0% <0%> (ø) ⬆️
editor/edit-post/header/index.js 0% <0%> (ø) ⬆️
... and 17 more

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 2056a3c...3f0ff9a. 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.

Nice Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants