paypal / glamorous

DEPRECATED: šŸ’„ Maintainable CSS with React
https://glamorous.rocks
MIT License
3.64k stars 208 forks source link

Can't set width/height on `canvas` components #413

Closed dylanpyle closed 6 years ago

dylanpyle commented 6 years ago

I initially thought this was the same issue as #412, but it doesn't look like it.

It's not possible to set width/height props on <canvas> components as far as I can tell, since they're being blacklisted in should-forward-property.js here: https://github.com/paypal/glamorous/blob/75495fd4bd1083d8fc0d12fa805188da759ac03b/src/should-forward-property.js#L20

I assume this has to do with the fact that these are also CSS properties, but unfortunately in the case of Canvas elements the "width" in the DOM and the CSS width have different meanings. The width/height attributes of the canvas DOM node control the coordinate space of the drawing area that you use e.g. drawImage to write to later on, and the CSS properties don't affect that.

Using:

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

Relevant code.

const Canvas = glamorous.canvas();
render(<Canvas width='200' height='300' />, el);

Actual result:

<canvas class="css-glamorous-canvas--uem9b8"></canvas>

Expected result:

<canvas class="css-glamorous-canvas--uem9b8" width="200" height="300"></canvas>

https://codesandbox.io/s/pyo7ypwx4q

Ailrun commented 6 years ago

Thank you for reporting the problem! Actually, img also make same (or at least similar) problem, since width/height of img tag has different semantics.

In my opinion, we should add some special props name for attribute-forwarded width/height, for backward compatibility (not to change the role of original width and height css props).

@kentcdodds, how do you think about?

kentcdodds commented 6 years ago

This is probably an issue for https://github.com/wooorm/html-tag-names or https://github.com/jackyho112/react-html-attributes šŸ¤”

dylanpyle commented 6 years ago

@kentcdodds That was my first assumption given the similarity to #412, but if you check out the link to should-forward-property.js above, it looks like it's a blacklist of props in this repo itself. Maybe I'm missing something obvious but react-html-attributes seems to include width/height on canvas as expected.

kentcdodds commented 6 years ago

Ah, yes, you're correct. I do remember making that blacklist. I think there are similar issues with SVGs as there are with canvas. Perhaps we should add an isCanvas function and add that check here:

https://github.com/paypal/glamorous/blob/75495fd4bd1083d8fc0d12fa805188da759ac03b/src/should-forward-property.js#L65

kentcdodds commented 6 years ago

This project is now in an unmaintained status. Please see the README for more information.