fix: legacy image migration #975
Conversation
kellyjosephprice
requested review from
kevinports,
rafegoldberg and
trishaprile
| @@ -19,6 +20,12 @@ const compatibilityTransfomer = (): Transform => tree => { | |||
| return SKIP; | |||
| }); | |||
|
|
|||
| visit(tree, 'image', (node: Image, index: number, parent: Parent) => { | |||
| if (phrasing(parent) || !parent.children.every(child => child.type === 'image' || !phrasing(child))) return; | |||
There was a problem hiding this comment.
Wait, not sure I understand what would count as phrasing content? Would it match this image:
Lorem  ipsum.And does that mean we'd be treating/displaying ^said image as a block? That doesn't seem right…
There was a problem hiding this comment.
And also, would this image still float to the right?
Lorem ipsum:
<img align=right src=my.img />
Dolor set amos.There was a problem hiding this comment.
And does that mean we'd be treating/displaying ^said image as a block? That doesn't seem right…
We've been treating images that are alone on a line as a readme-style block level image for sometime:
## Block Image
Lorem ipsum.
And also, would this image still float to the right?
Yea, that would be parsed as mdxJsxFlowElement. The image node type is usually only used for markdown images. RDMD would convert magic blocks to image nodes for simplicity. But for the reasons you're asking about, was probably a huge mistake.
There was a problem hiding this comment.
I guess, should we use this migration to convert lone markdown images to JSX, to remove the ambiguity?
There was a problem hiding this comment.
And does that mean we'd be treating/displaying ^said image as a block? That doesn't seem right…
THis logic should ignore that image then. It's checking to make sure that all of the images siblings aren't phrasing. eg:
## Heading
> 
>
> - all block components
| @@ -19,6 +20,12 @@ const compatibilityTransfomer = (): Transform => tree => { | |||
| return SKIP; | |||
| }); | |||
|
|
|||
| visit(tree, 'image', (node: Image, index: number, parent: Parent) => { | |||
| if (phrasing(parent) || !parent.children.every(child => child.type === 'image' || !phrasing(child))) return; | |||
There was a problem hiding this comment.
So if I'm understanding this thread correctly, "phrasing content" means an inline image? (I mainly ask 'cause I was noticing some issues1 with inline image editing earlier today and am wondering if this might help!)
Footnotes
-
namely that the editor was auto-deleting all inline
<Images>and! 😬 ↩
There was a problem hiding this comment.
Do you have a link to a page?
There was a problem hiding this comment.
T'was just a test page on the backup DB, but essentially this:
Lorem ipsum dolor sit amet. <Image src="some.img" /> Consectetur elit neque.There was a problem hiding this comment.
Ok, working on that in a separate PR.
There was a problem hiding this comment.
Shit, sorry, I approved too quickly! I forgot about this question; I actually think that needs investigating/answering before we merge this.
## Version 7.6.5 ### 🛠 Fixes & Updates * copy button icons ([#982](#982)) ([bbe2060](bbe2060)) * fallback for 5 ([#984](#984)) ([c1ad0ac](c1ad0ac)), closes [#c50a50](https://github.com/readmeio/markdown/issues/c50a50) * inline mdx images ([#983](#983)) ([3faa95d](3faa95d)) * legacy image migration ([#975](#975)) ([c900d5f](c900d5f)) <!--SKIP CI-->
|
This PR was released!🚀 Changes included in v7.6.5 |
🧰 Changes
Fixes migrating images with legacy data.
We deprecated a particular format of image magic block, but we failed to ever migrate pages away from it. They've been broken/changed for awhile, and I'm not sure if we could correctly detect the format at this point?
When they get parsed into an AST, they appear to be inline image nodes, which can then compile incorrectly. We've been treating image nodes that are alone in a paragraph as block images for awhile, so lets encode that in the migration.
🧬 QA & Testing