-
Notifications
You must be signed in to change notification settings - Fork 10
fix: embeds and images save better #893
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
Conversation
echo "${NODE_ENV}" > public/data/build-env && \ | ||
npx ts-node ./bin/print-webpack-config.ts > ./build-time-webpack-config.json && \ | ||
npm run ui | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hee hee
// url, | ||
// }; | ||
|
||
// parent.children[index] = mdNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kellyjosephprice i commented out some of your image stuff because it was gettin' rull weird with my changes and i figured we could work it out later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means 'complexImages' won't render as the image widget in the editor, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for embeds, I think we need to convert them from JSX to mdast/slate-y type nodes for the editor to pick them up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it works in the editor playground, but i haven't checked in a project yet (doing so now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change the playground source to:
<Image align="center" className="border" url="https://drastik.ch/wp-content/uploads/2023/06/blackcat.gif" />
does it load as a widget or as JSX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OH I think it's falling through to here:
markdown/processor/transform/readme-components.ts
Lines 97 to 111 in ef1c17b
const hProperties = attributes(node); | |
// @ts-ignore | |
const mdNode: CodeTabs = { | |
children: node.children as any, | |
type: types[node.name], | |
...(['tr', 'td'].includes(node.name) | |
? {} | |
: { | |
data: { | |
hName: node.name, | |
...(Object.keys(hProperties).length ? { hProperties } : {}), | |
}, | |
}), | |
position: node.position, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add embeds to this map:
markdown/processor/transform/readme-components.ts
Lines 9 to 19 in ef1c17b
const types = { | |
Callout: NodeTypes['callout'], | |
Code: 'code', | |
CodeTabs: NodeTypes['codeTabs'], | |
Glossary: NodeTypes['glossary'], | |
Image: 'image', | |
Table: 'table', | |
Variable: NodeTypes['variable'], | |
td: 'tableCell', | |
tr: 'tableRow', | |
}; |
And then they'll get picked up in the editor?
// url, | ||
// }; | ||
|
||
// parent.children[index] = mdNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means 'complexImages' won't render as the image widget in the editor, right?
// url, | ||
// }; | ||
|
||
// parent.children[index] = mdNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for embeds, I think we need to convert them from JSX to mdast/slate-y type nodes for the editor to pick them up.
return memo; | ||
}, {} as T); | ||
|
||
export const isMDXElement = (node: Node): node is MdxJsxFlowElement | MdxJsxTextElement => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still reviewing, but the only thing I'm seeing in the app is that the compilers aren't adding newlines?
this still needs some cleanup, but i'd like to get this merged ASAP for QA purposes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
@@ -1,6 +1,6 @@ | |||
import { mdast } from '../index'; | |||
|
|||
describe.skip('Parse html block', () => { | |||
describe('Parse html block', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
@@ -1,12 +1,12 @@ | |||
export enum NodeTypes { | |||
callout = 'rdme-callout', | |||
codeTabs = 'code-tabs', | |||
embed = 'embed', | |||
embedBlock = 'embed-block', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a regular embed block still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, no. i changed the structure enough that the serializing/deserializing was getting to be spaghetti with the current/old embed
block, so i just moved it all wholesale to a new block/block type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it also allows me more freedom to dial in the new image and embed blocks, haha
This PR was released!🚀 Changes included in v6.75.0-beta.53 |
🧰 Changes
embeds and images had a few kinks still, and they've been worked out? mostly? (🤞🏻)
🧬 QA & Testing