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

<em-emoji> size prop does not work if a number is passed in #656

Closed
srmagura opened this issue Jul 27, 2022 · 3 comments
Closed

<em-emoji> size prop does not work if a number is passed in #656

srmagura opened this issue Jul 27, 2022 · 3 comments

Comments

@srmagura
Copy link

srmagura commented Jul 27, 2022

// This creates a large emoji
;<em-emoji id="+1" size="100px" />

// These create normal size emojis
;<em-emoji id="+1" size="100" />
;<em-emoji id="+1" size={100} />  // in JSX

It's because element.getAttribute('size') returns '100' here. Then you render the element with style={{ fontSize: '100' }} in components/Emoji/Emoji.js. If it were style={{ fontSize: 100 }}, it would make the emoji large as expected.

I'm not sure if this is an oversight or working as intended. Regardless, I think it would be better if the size="100" syntax was supported. This would match what people expect based on normal HTML elements, e.g. <img src="..." width="100"> creates a 100px-wide image.

Let me know if you want a PR.

P.S. Thank you for all the recent bug fixes! 🎉

@EtienneLem
Copy link
Member

The size prop ends up being used in an inline style attribute (CSS), so that’s why 100 alone doesn’t do anything, unit-less value isn’t supported in CSS.

We’d need to parse the provided value and detect that no unit is given (maybe if only digits with a regex?), then we’d assume px and force it. Would need to be handled at the “schema” level (in getProp you linked) so that on this line there’s never a unit-less size value.

@srmagura
Copy link
Author

The size prop ends up being used in an inline style attribute (CSS), so that’s why 100 alone doesn’t do anything, unit-less value isn’t supported in CSS.

I'm splitting hairs but, the code is using the style prop in Preact. Assuming Preact works the same way as React, unitless values are supported if it's a number type.

I think detecting unitless values using a regex and then appending px is a good idea.

@EtienneLem
Copy link
Member

I'm splitting hairs but, the code is using the style prop in Preact. Assuming Preact works the same way as React, unitless values are supported if it's a number type.

Not quite. Even with React/Preact/JSX, <em-emoji> is an HTML component. HTML attributes are always a string (read with getAttribute).

So yeah, the only fix was to assume px when no unit is provided.

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

No branches or pull requests

2 participants