Skip to content

Conversation

@jennspencer
Copy link
Contributor

@jennspencer jennspencer commented Apr 9, 2024

PR App Fix RM-9020

🧰 Changes

MDXify the Image component

🧬 QA & Testing

@jennspencer jennspencer changed the title Jenn/rm 9020 convert image blocks to mdx feat: image components! Apr 9, 2024
@jennspencer jennspencer self-assigned this Apr 9, 2024
Copy link
Contributor

@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.

This looks good. But I think we need to replace the magic block stuff with some equivalent MDX?

Also, would you try enabling tests in these files:

__tests__/components/test.js
__tests__/flavored-compilers/images.test.js
__tests__/index.test.js

@jennspencer
Copy link
Contributor Author

@kellyjosephprice okay that test is being a pain and we can address it later :P

Copy link
Contributor

@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.

Mostly LGTM, accept for the compiler's using the old style syntax.

Comment on lines 9 to 17
visitors.image = function compile(node: Image, ...args: any[]): string {
if (node.data?.hProperties?.className === 'emoji') return node.title;

const { align, className, width } = node.data?.hProperties || {};
const complexImage: boolean = Boolean(width) || Boolean(className) || Boolean(align);
if (complexImage) return `<Image ${{...node.data?.hProperties}} />`;

return originalImageCompiler.call(this, node, ...args);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I wasn't aware of this 3 weeks ago...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hah! awesome--i had seen the syntax change but wasn't sure if that was just gemoji related

Copy link
Contributor

Choose a reason for hiding this comment

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

It's like, well typed, but I could only figure it out by reading the remark source. 😖

"release.dry": "npx semantic-release --dry-run",
"start": "webpack serve --open --mode development --config ./webpack.dev.js",
"test": "vitest",
"test.ui": "vitest --ui",
Copy link
Contributor

Choose a reason for hiding this comment

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

:neat:

"node_modules",
"dist"
]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

my json formatter has opinions apparently

Copy link
Contributor

@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.

@jennspencer
Copy link
Contributor Author

LGTM!

🙏 Would you uncomment this line: https://github.com/readmeio/markdown/blob/beta/__tests__/browser/markdown.test.js#L20

Then run make updateSnapshot

heh we have dueling default fonts

@kellyjosephprice
Copy link
Contributor

heh we have dueling default fonts

Ugh, docker was supposed to fix this.

@jennspencer
Copy link
Contributor Author

heh we have dueling default fonts

Ugh, docker was supposed to fix this.

haha i know, i just tried to cheat and not run it in a container but then i gave in

@jennspencer jennspencer merged commit 0e015ce into beta May 9, 2024
@jennspencer jennspencer deleted the jenn/rm-9020-convert-image-blocks-to-mdx branch May 9, 2024 00:02
rafegoldberg pushed a commit that referenced this pull request May 9, 2024
## Version 6.75.0-beta.29

### ✨ New & Improved

* image components! ([#857](#857)) ([0e015ce](0e015ce))

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

This PR was released!

🚀 Changes included in v6.75.0-beta.29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants