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

#895 Adding an image to an empty rich text editor doesn't trigger the Image widget #926

Merged

Conversation

Dammmien
Copy link
Contributor

@Dammmien Dammmien commented Dec 15, 2017

Fixes #895 : Adding an image to an empty rich text editor doesn't trigger the Image widget.

- Summary

  • Improve RegExp
  • Do not fallback undefined alt to an empty string.
    Input will be filled with undefined label ( the youtube widget has this behavior ).

- Test plan

Add an image to an empty rich text editor

- Description for the changelog

  • Improve RegExp
  • Do not fallback undefined alt to an empty string
    Input will be filled with undefined label ( the youtube widget has this behavior ).

@verythorough
Copy link
Contributor

verythorough commented Dec 15, 2017

Deploy preview ready!

Built with commit 745c4e2

https://deploy-preview-926--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Dec 15, 2017

Deploy preview ready!

Built with commit 745c4e2

https://deploy-preview-926--cms-demo.netlify.com

@@ -7,9 +7,9 @@ const image = {
image: match[2],
alt: match[1],
},
toBlock: data => `![${ data.alt || '' }](${ data.image || '' })`,
toBlock: data => `![${ data.alt }](${ data.image || '' })`,
toPreview: (data, getAsset) => <img src={getAsset(data.image || '')} alt={data.alt || ''} />,
Copy link
Contributor

@tech4him1 tech4him1 Dec 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erquhart I noticed that you modified this line as well in your 1.0 changes -- I had it as src={getAsset(data.image) || ''}. Is that something that needs reverted/fixed as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tech4him1 good catch, yes, that was unintentional.

@@ -7,9 +7,9 @@ const image = {
image: match[2],
alt: match[1],
},
toBlock: data => `![${ data.alt || '' }](${ data.image || '' })`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dammmien I'm just wondering why we want it to be undefined instead of an empty string, since that was just implemented recently. I'm not saying it's wrong, I'd just like a little background on why you can to change it.

Copy link
Contributor

@erquhart erquhart Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think his reasoning lies in comparison to the youtube component:

Input will be filled with undefined label ( the youtube widget has this behavior ).

But yeah I agree, should be empty string. Have a commit going that I'll be pushing soon, should be good to merge after that.

@erquhart erquhart force-pushed the fix/add_image_empty_rich_text_editor branch from df63859 to 745c4e2 Compare December 21, 2017 19:26
Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dammmien thanks for this! Quick review of my changes:

  • the dot operator captures everything except line breaks, but \S\s captures everything plus line breaks. Since line breaks aren't needed in alt text or the url, I've switched to the dot operator.
  • Implemented the fix pointed out by @tech4him1, where the OR operator was meant to be outside of the getAsset call args.

@erquhart erquhart merged commit 8849953 into decaporg:master Dec 21, 2017
@Dammmien Dammmien deleted the fix/add_image_empty_rich_text_editor branch April 26, 2018 20:15
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.

4 participants