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.

Sign up for GitHub

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

Jump to bottom

feat: embeds! #878

Merged
merged 12 commits into from
May 20, 2024
Merged

feat: embeds! #878

merged 12 commits into from
May 20, 2024

Conversation

jennspencer
Copy link
Contributor

@jennspencer jennspencer commented May 17, 2024 •
edited
Loading

icn Fix RM-9017

🧰 Changes

embeds!!!

🧬 QA & Testing

still need to make updateSnapshot

@jennspencer
Copy link
Contributor Author

@kellyjosephprice i dinked around a lot with types and tests here, which looks like it caused some conflicts 😂 i'll muddle through them but will defer to whatever patterns you're using

kellyjosephprice
kellyjosephprice requested changes May 18, 2024
View reviewed changes
Copy link
Collaborator

@kellyjosephprice kellyjosephprice left a comment

Choose a reason for hiding this comment

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

LGTM. The only one that really concerns me is the error.

processor/transform/embeds.ts Outdated Show resolved Hide resolved
types.d.ts Show resolved Hide resolved
processor/transform/embeds.ts Outdated
return (tree: any) => {
visit(tree, 'link', (node, _, parent) => {
try {
if (parent.children.length > 1 || node.title !== '@embed') return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we should check if the parent is a paragraph, and completely replace it?

Copy link
Contributor Author

@jennspencer jennspencer May 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yeah i should still be checking if the parent is a paragraph, i think i uh optimized that out at one point, heh. but i'm basically just reforming the node and replacing the parent with it--should i be splicing it in and out or something instead?

@kellyjosephprice
Copy link
Collaborator

I think I fixed the syntax highlighter dynamic imports on the beta branch

@jennspencer
Copy link
Contributor Author

@kellyjosephprice the Code component was/is working already, yeah? these errors are just on the test i edited so there's likely more tweaks it needs. the snapshot not matching is odd tho, wtf?

we can add a couple of todos to fix these guys, it's the last "convert component" ticket and i'd like to get crackin' on the editor-side component(s), whatchoo say?

@kellyjosephprice
Copy link
Collaborator

@kellyjosephprice the Code component was/is working already, yeah? these errors are just on the test i edited so there's likely more tweaks it needs. the snapshot not matching is odd tho, wtf?

we can add a couple of todos to fix these guys, it's the last "convert component" ticket and i'd like to get crackin' on the editor-side component(s), whatchoo say?

Yea, it seems like there's something weird happening with the compilation step and the dynamic import?

Skip it!

@jennspencer
Copy link
Contributor Author

@kellyjosephprice the Code component was/is working already, yeah? these errors are just on the test i edited so there's likely more tweaks it needs. the snapshot not matching is odd tho, wtf?
we can add a couple of todos to fix these guys, it's the last "convert component" ticket and i'd like to get crackin' on the editor-side component(s), whatchoo say?

Yea, it seems like there's something weird happening with the compilation step and the dynamic import?

Skip it!

YOLO

@jennspencer jennspencer merged commit 9dee309 into beta May 20, 2024
0 of 11 checks passed
@jennspencer jennspencer deleted the jenn/rm-9017-convert-embeds-to-mdx branch May 20, 2024 20:14
rafegoldberg pushed a commit that referenced this pull request May 20, 2024
@semantic-release-bot
build(release): 🚀 v6.75.0-beta.34 🦉
2347ac2 
## Version 6.75.0-beta.34

### ✨ New & Improved

* embeds! ([#878](#878)) ([9dee309](9dee309))

<!--SKIP CI-->
@rafegoldberg
Copy link
Contributor

This PR was released!

🚀 Changes included in v6.75.0-beta.34

@kellyjosephprice kellyjosephprice mentioned this pull request Jul 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kellyjosephprice kellyjosephprice Awaiting requested review from kellyjosephprice

Assignees
No one assigned
Labels
released on @beta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jennspencer @kellyjosephprice @rafegoldberg