Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Doesn't forward playsInline prop to <video> components #412

Closed
dylanpyle opened this issue Apr 18, 2018 · 5 comments
Closed

Doesn't forward playsInline prop to <video> components #412

dylanpyle opened this issue Apr 18, 2018 · 5 comments

Comments

@dylanpyle
Copy link

dylanpyle commented Apr 18, 2018

Using:

"glamor": "2.20.40",
"glamorous": "4.12.3",
"react": "16.3.2",

Relevant code.

const Video = glamorous.video();
render(<Video playsInline />, el);

Actual result:

<video class="css-glamorous-video--irvhhv"></video>

Expected result:

<video class="css-glamorous-video--irvhhv" playsinline></video>

https://codesandbox.io/s/8k2wx5p3m0


Assuming this is a whitelist in the code somewhere, I'll try to follow up with a PR shortly.

@dylanpyle
Copy link
Author

dylanpyle commented Apr 18, 2018

Looks like this comes from the upstream project react-html-attributes. Opened a PR here: jackyho112/react-html-attributes#9

I am curious though - is there a possibility of removing this whitelist? Seems like it's destined to get out of sync as things change in React. Is there a downside in just forwarding all unknown props?

@kentcdodds
Copy link
Contributor

The problem is if we forward all the props then we could send props to dom nodes that don't actually belong there and can cause some issues (and React will warn against it, resulting in many console log statements).

@kentcdodds
Copy link
Contributor

Glad you got it fixed!

@dylanpyle
Copy link
Author

Yeah, interesting — I guess I figured that was more of the user's responsibility to avoid, but I see how it's a potential footgun.

This issue is fixed upstream, but looks like the NPM distribution of glamorous uses dist/glamorous.cjs.js as the entry point, so I don't think it'll be fixed on NPM until you republish. Not sure how your usual release cycle works, so feel free to close this issue whenever.

@kentcdodds
Copy link
Contributor

Ah, yes, that's correct. I can fix that right now.

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

No branches or pull requests

2 participants