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

Replace unsupported className prop with class in TypeScript typings #131

Merged
merged 1 commit into from
Mar 31, 2021
Merged

Replace unsupported className prop with class in TypeScript typings #131

merged 1 commit into from
Mar 31, 2021

Conversation

NMinhNguyen
Copy link
Contributor

See https://reactjs.org/docs/web-components.html#using-web-components-in-react

Although this change can be viewed as breaking, I wouldn't personally call it such - if you pass className, then the class doesn't get applied. This is a bug fix in my opinion, and it allows consumers to correctly pass class prop.

@timkpaine
Copy link
Member

@texodus are the typings formally supported at all? Or a should we drop typings altogether

@telamonian
Copy link
Contributor

are the typings formally supported at all? Or a should we drop typings altogether

@timkpaine They are formally supported, by me. They do need a refresh currently for eg the recent changes to setDataListener. I'll take a look at this PR

@telamonian
Copy link
Contributor

@NMinhNguyen Can you please add some more details as to why the change in this PR is needed? I read through the doc you linked, but it didn't really shed any light, since there's no mention of typescript on that page

@NMinhNguyen
Copy link
Contributor Author

NMinhNguyen commented Mar 31, 2021

@NMinhNguyen Can you please add some more details as to why the change in this PR is needed? I read through the doc you linked, but it didn't really shed any light, since there's no mention of typescript on that page

I've replied to your comment, but this is the main thing to take away:

One common confusion is that Web Components use “class” instead of “className”.

In other words, <regular-table> does not support className prop, irrespective of TypeScript or JavaScript. What it does support is class, but the current typings prevent me from passing that and I have to use @ts-expect-error in order to silence TypeScript.

@texodus
Copy link
Member

texodus commented Mar 31, 2021

A somewhat related unresolved issue which we've encountered in Perspective is how this property conflicts when a Web Component is used in React. Has this been resolved in more recent React versions? The official docs does not seem to acknowledge any known issues when class is used in a JSX template on a Custom Element.

@texodus texodus merged commit 1fb71b5 into finos:master Mar 31, 2021
@NMinhNguyen NMinhNguyen deleted the class branch April 1, 2021 07:17
@NMinhNguyen
Copy link
Contributor Author

NMinhNguyen commented Apr 1, 2021

A somewhat related unresolved issue which we've encountered in Perspective is how this property conflicts when a Web Component is used in React. Has this been resolved in more recent React versions? The official docs does not seem to acknowledge any known issues when class is used in a JSX template on a Custom Element.

The official docs explicitly say to use classand not className. You can refer to facebook/react#4933 (comment) for more context.

The issue you link to seems a bit misguided - they use Enzyme as motivation, but I'd argue that that's Enzyme's fault for expecting a certain React prop - with React Testing Library and Jest DOM matchers you could do expect(element).toHaveClass('disabled') and it would just work.

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

Successfully merging this pull request may close these issues.

4 participants