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

[Switch] inputProps can take additional props #18902

Closed
wants to merge 2 commits into from

Conversation

Natalia504
Copy link

This change allows for additional attributes to be forwarded to the input element via inputProps.
This currently exists in InputBaseComponentProps here: https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/InputBase/InputBase.d.ts#L54-L58

@Natalia504 Natalia504 changed the title [Switch Component] inputProps can take additional props [Switch] inputProps can take additional props Dec 17, 2019
@mui-pr-bot
Copy link

No bundle size changes comparing 192b426...b725602

Generated by 🚫 dangerJS against b725602

@Natalia504
Copy link
Author

@oliviertassinari , here's sandbox for the issue that this PR is resolving:
https://codesandbox.io/s/material-demo-f6m52?fontsize=14&hidenavigation=1&module=%2Fdemo.tsx&theme=dark
Would this suffice?

@mbrookes mbrookes requested a review from eps1lon December 18, 2019 00:49
@mbrookes mbrookes added component: switch This is the name of the generic UI component, not the React module! typescript labels Dec 18, 2019
@oliviertassinari
Copy link
Member

Is this related to #18874? It sounds like something we should have a systematic solution for, not a one case exception.

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 18, 2019
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.

Could you add a test to showcase what this fixes? It's not obvious to me why we need this change.

@@ -21,6 +21,11 @@ export interface SwitchBaseProps
value?: unknown;
}

export interface SwitchBaseComponentProps extends React.HTMLAttributes<HTMLInputElement> {
// Accommodate arbitrary additional props coming from the `inputProps` prop
[arbitrary: string]: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that this will not catch excess props that are actually wrong (such as those from typos or accidental wrong names.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 27, 2019

The codesandbox shows the following pattern:

<Switch
  checked={checked}
  onChange={toggleChecked}
  inputProps={{ "data-testid": "test" }}
/>

The pull request seems to apply the same solution to the SwitchBase as we use with the InputBase:
https://github.com/mui-org/material-ui/blob/e59dbe3b0da43414dc64a660015f49b21c391c06/packages/material-ui/src/InputBase/InputBase.d.ts#L55-L58


@eps1lon On one hand, it would be more consistent, but on the other hand, we have microsoft/TypeScript#28960 and the fact that we encourage custom input component implementation that we don't with the switch, radio, and checkbox.

What would be best?

@eps1lon
Copy link
Member

eps1lon commented Dec 27, 2019

I need to think about this more. I think I'd rather encourage explicit any casting (not adding [arbitrary: string]: any;) so that you're responsible about usage rather than doing it all the time with some false passes.

@afholderman
Copy link

@eps1lon @oliviertassinari @Natalia504

What do you think of adding something like this to the index.d.ts:

export type StandardInputProps<E = React.InputHTMLAttributes<HTMLInputElement>> = E | {
  [arbitrary: key]: any;
}

Then for instance inputProps could be changed to StandardInputProps here and StandardInputProps<React.InputHTMLAttributes<HTMLInputElement | HTMLTextAreaElement>> here.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 3, 2020

@afholderman I believe the InputBase case is a bit different to the SwitchBase, it's not uncommon to use a different implementation of the <input> element.

@oliviertassinari
Copy link
Member

Maybe we should give up on the changes, and ask developers to manually cast?

@afholderman
Copy link

@oliviertassinari passing data attributes down is a supported use case by React so we should try to find a solution for this issue. Especially with growing adoption of testing-library as a hooks compatible replacement for enzyme which supports the use of data-testid to query the DOM.

@oliviertassinari
Copy link
Member

@afholderman Why using a test-id over the accessible feature of the element (name, label, etc)?

@oliviertassinari
Copy link
Member

It has been a month. I believe we can close the pull request with #18902 (comment) as a recommended approach :).

@t0wer001
Copy link

Just ran into this issue. And even from the suggestions here I am not exactly sure how to get around it.

Do you have an example with the casting explicit any as mentioned in #18902 (comment)

My current work around by applying the data-testid directly on the component and then acces the child nodes until I reach the input:

component:

<Switch
  data-testid="my-test-id"
/>

test:

clickSwitch: () => {
  const wrapperNode = getByTestId('my-test-id')
  const switchNode = wrapperNode.childNodes[0].childNodes[0]

  fireEvent.click(switchNode as HTMLElement)
},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: switch This is the name of the generic UI component, not the React module! PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants