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

Stringify another array property #715

Merged
merged 2 commits into from
Aug 6, 2019
Merged

Stringify another array property #715

merged 2 commits into from
Aug 6, 2019

Conversation

karlhorky
Copy link
Contributor

Building on @devongovett's #158, this adds the sandbox property to the list of properties that should allow spaces.

Without this fix, using gatsby-remark-embedded-codesandbox along with gatsby-plugin-mdx results in an iframe with a sandbox attribute of allow-modals,allow-forms,allow-popups,allow-scripts,allow-same-origin, even though gatsby-remark-embedded-codesandbox is correctly returning space-separated strings.

If this is truly the correct solution, then probably we should include other HTML properties that accept spaces.

If there's a better / more correct way of doing this, I'm all ears! The remark plugin gatsby-remark-embedded-codesandbox does appear to be doing everything right however, returning a node.value with a sandbox attribute separated by spaces. It seems like MDX itself is changing the spaces to commas...

Fixes elboman/gatsby-remark-embedded-codesandbox#7

cc @timneutkens @elboman

@vercel
Copy link

vercel bot commented Aug 6, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://mdx-git-fork-karlhorky-patch-1.mdx.now.sh

@wooorm
Copy link
Member

wooorm commented Aug 6, 2019

@johno Is there a reason https://github.com/syntax-tree/hast-to-hyperscript isn’t used? It does exactly all this for both React and Vue!

@timneutkens
Copy link
Member

timneutkens commented Aug 6, 2019

I remember it does too much conversion. We wanted the available properties for users to override to be MDAST elements not HAST elements.

Eg there's no inlineCode etc.

@vercel vercel bot temporarily deployed to staging August 6, 2019 16:10 Inactive
@wooorm
Copy link
Member

wooorm commented Aug 6, 2019

Ahh, right, this is MDXHAST, not hast 🤔

We could move the attribute handling in hast-to-hyperscript to its own file, but I think you’ll then pull in more weight that needed.

Alternatively, I’d suggest to depend on property-information and inline the code in https://github.com/syntax-tree/hast-to-hyperscript/blob/d2a1b3d5336beca8ee6b68e8cefe46efeac71e93/index.js#L148-L198 (and remove all the unneeded non-react things)

@johno
Copy link
Member

johno commented Aug 6, 2019

Yeah, I think we'd need a handful of changes so it'd work the way we want for MDX. That said, at the very least, we should break out a lot of this shared code into a package/file these libraries could share. Though we should probably make that it's own issue.

I'm cool merging this as is since it fixes something that's indeed broken while we investigate how we can better implement the MDXHAST to JSX conversion.

@wooorm
Copy link
Member

wooorm commented Aug 6, 2019

Yup, agreed!

@johno
Copy link
Member

johno commented Aug 6, 2019

Cools, will do. Thanks for the PR @karlhorky!

@karlhorky
Copy link
Contributor Author

No worries, thanks for the merge / release!

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

Successfully merging this pull request may close these issues.

I'm only getting a black box for embed
4 participants