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

Add image (and void block support) to the editor #84

Merged
merged 54 commits into from
Jun 23, 2021
Merged

Conversation

ktrieu
Copy link
Collaborator

@ktrieu ktrieu commented Nov 7, 2020

This PR adds support for images, and void blocks more generally, to the editor.

This ended up being a little more complicated than I thought because of the difficulties of inserting paragraphs before/after void blocks. To solve this, we use void spacers, which are void blocks that we automatically insert before and after void content blocks. These provide "anchors" for the selection to grab and insert paragraphs in.

I've said this before, but this is one of the last architectural issues to solve in the editor. We have now covered all four permutations of the block/inline void/not void universe. Addition of new, fancy block types should be much simpler now.

The image block here just take a link as a property and show an <img> element. If the image doesn't load, we show a nice error block instead of a broken tag.

Kevin Trieu added 30 commits September 22, 2020 22:25
In particular, we have to render props.children, spread attributes, and still make the img not editable.
Now that normalization handles removing empty lists, we can remove that logic from the list break code.
…eparate file

This should allow us to insert paragraphs before/after void blocks in a more natural way.
Styling subject to change, of course.
@ktrieu
Copy link
Collaborator Author

ktrieu commented Apr 16, 2021

Fixed your comments, but we're waiting on ianstormtaylor/slate#4050 upstream to fix that last security issue.

@ktrieu
Copy link
Collaborator Author

ktrieu commented Apr 16, 2021

I was thinking about the extra image URL checks you mentioned, and I realized that if we stuck with our "upload to Imgur" policy for images, we'd basically be hotlinking en masse when the website goes public, which is against Imgur TOS, and probably a lot of other places.

The answer here is probably "build an image uploading system" for our server, which would make the URL checks you mention unecessary. Thoughts?

@tyxchen
Copy link
Member

tyxchen commented Apr 17, 2021

An image uploading system makes me skittish because file upload functionality is a common source of major security issues, in addition to opening up the possibility of abuse.

I wasn't aware that Imgur's TOS prohibited hotlinking. This will be a topic I need to put more thought into.

@ktrieu
Copy link
Collaborator Author

ktrieu commented Apr 17, 2021

The other option is some sort of service like S3, which would save us development time and alleviate those security issues, at the cost of having to manage the hosting service instead, plus some monthly cost, which will be annoying financially.

@tyxchen
Copy link
Member

tyxchen commented Apr 17, 2021

Yeah, the cost of S3 increases over time (pricing is based on amount stored + number of accesses). I also don't know how payment would work out, and long-term there's a pretty high risk that this cost might be forgotten or overlooked by future editorial teams, which could lead to some nasty surprises.

@tyxchen tyxchen added upstream Requires a change from upstream and removed upstream Requires a change from upstream labels May 21, 2021
@tyxchen
Copy link
Member

tyxchen commented May 21, 2021

@ktrieu It looks like the slate security issue is fixed

@ktrieu
Copy link
Collaborator Author

ktrieu commented May 22, 2021

Looks like we have another, different security problem: facebook/create-react-app#10945

CRA uses PostCSS 7, which has a security problem which PostCSS doesn't want to patch because they are now supporting PostCSS 8 only. I figure I had about a five-day window to merge this between the Slate thing being fixed and the CRA thing breaking.

This updated Slate which broke a few things, more work needed.
@tyxchen
Copy link
Member

tyxchen commented May 22, 2021

Ugh.

I'm beginning to wonder if we should drop CRA completely. It seems to just be giving us headache after headache, for really not all that much benefit.

ktrieu added 2 commits May 22, 2021 20:57
Tests still don't work, more to come.
Honestly, I'm not sure how these worked before.
@tyxchen
Copy link
Member

tyxchen commented Jun 20, 2021

So it looks like a whole bunch of RegEx DoS issues got found/discovered recently. Ugh.

At this point I'm inclined to just merge and get this over with, thoughts?

@ktrieu
Copy link
Collaborator Author

ktrieu commented Jun 20, 2021

I think we can mitigate this kind of thing by moving react-scripts to devDependencies and switching the security check command to npm audit --production. Then, we can skip these warnings which don't really represent vulnerabilities in our application while hopefully still being informed of actual ones that may come up.

I tried that, but there are still some vulnerabilities in some other packages. Since this PR is so huge and it's been stuck for so long, let's just merge it and I'll make those changes and deal with those other vulnerabilities in another PR.

@tyxchen
Copy link
Member

tyxchen commented Jun 23, 2021

Builds are failing because we moved src/styles/slugline.scss to src/styles/primary_style.scss, src/styles/_slugline_vars.scss to src/styles/_primary_style_vars.scss, etc. Probably want to update that before merge.

@ktrieu ktrieu merged commit a0c48af into master Jun 23, 2021
@ktrieu ktrieu deleted the image-support branch June 23, 2021 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request unit: editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants