Pass through props solve #34

[sag1v]

Passing none API props to the underline input element. Should solve #34

[markusenglund]

Alright nice. I'm gonna take closer look at it later, but an initial thought is that we could just remove the code for a couple of props like name, aria-label etc since they're basically just passed on to the input.

[sag1v]

Yeah indeed. Its not breaking the API but it will require a small change to the docs, and obviously the internal code. I didn't want to make that change without your approval.

Another thing, we need to remember to filter out all the lib related props in render before we pass ...rest to the input. Otherwise react will complain about none standard DOM attributes.

[markusenglund]

True. I think the tests should hopefully fail if a library related prop is allowed to be passed to the input.

[sag1v]

BTW i forgot to mention, i added cross-env and changed the script that needs it. because i couldn't run some of the scripts which uses NODE_ENV (i'm using windows). This should normalize the execution for windows and linux users. Hope that's OK with you.

[sag1v]

onChange gets passed to the input through the rest syntax, which is causing a glitch when the user tries to toggle with space-bar. It should be filtered out like you did with handleDiameter

What do you say, instead of filtering handlers we should just bump them down order wise. this way we make sure they wont get overridden.

<input
  type="checkbox"
  role="switch"
  checked={checked}
  disabled={disabled}
  style={inputStyle}
  ref={this.$getInputRef}
  {...rest}
  /* anything below should NOT get overridden by ...rest */
  onFocus={this.$setHasOutline}
  onBlur={this.$unsetHasOutline}
  onKeyUp={this.$onKeyUp}
  onChange={this.$onInputChange}
/>

All props that are just passed to the input should be passed through ...rest (ie name etc)

Yeah, except already destructured variables like disabled.

[markusenglund]

Yeah, ordering them like that works great! I feel like this is looking good now. The only thing left to do is to rework the documentation a little bit. I'm thinking we should probably remove some of these props from the api-list in the README (like name, but probably not checked), and instead write a paragraph that explains how any props that isn't listed is passed to the nested input.

Then there's the typescript definition file... I'm not familiar with typescript, so I don't know how if we have to make any changes there...

[sag1v]

Then there's the typescript definition file... I'm not familiar with typescript, so I don't know how if we have to make any changes there

I don't think we should (or can) describe the ...rest props as they are intended to be the standard HTML attributes. But we should remove all of the other props that we removed from render. My rule of thumb: if the prop is not used in our code-base it shouldn't be documented. so props like checked, id and onChange should be documented although they are standard attributes to the underline element.

[markusenglund]

Alright, that sounds good. I don't want to remove the aria-label/labelledby examples though. They illustrate good practice for using the component.

[sag1v]

I don't want to remove the aria-label/labelledby examples though. They illustrate good practice for using the component

Its back :)

[NoTrebel]

Then there's the typescript definition file... I'm not familiar with typescript, so I don't know how if we have to make any changes there

I don't think we should (or can) describe the ...rest props as they are intended to be the standard HTML attributes. But we should remove all of the other props that we removed from render. My rule of thumb: if the prop is not used in our code-base it shouldn't be documented. so props like checked, id and onChange should be documented although they are standard attributes to the underline element.

I just tried out this branch and enabled typescript (as I'd like to use this library, and use typescript). I do think the passed through props need to be described, otherwise you'd get errors like the following:

Type '{ onChange: (checked: any) => void; checked: boolean; tabIndex: string; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<ReactSwitch> & Readonly<ReactSwitchProps> & Readonly<{ children?: ReactNode; }>'.  

Property 'tabIndex' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<ReactSwitch> & Readonly<ReactSwitchProps> & Readonly<{ children?: ReactNode; }>'.ts(2322))

My suggestion would be to add the inputProps from React.InputHTMLAttributes<HTMLInputElement> and remove those that can't be used (like 'onBlur' as it's used internally):

type Omit<T, U> = Pick<T, Exclude<keyof T, U>>

type htmlInputProps = React.DetailedHTMLProps<React.InputHTMLAttributes<HTMLInputElement>, HTMLInputElement>
type excludedHTMLInputProps = 'onFocus' | 'onBlur' | 'onKeyUp' | 'onChange' | keyof ReactSwitchProps

type allowedHTMLinputProps = Omit<htmlInputProps, excludedHTMLInputProps>

declare class ReactSwitch extends React.Component<ReactSwitchProps | allowedHTMLinputProps> {}

I've used a slightly non-standard variant of Omit, to make it easier to omit props by their name only (and allow props that don't exist on input in the first place to be omitted).

[markusenglund]

Included in 5.0.0 which was just released!

[markusenglund]

@NoTrebel Thanks for discovering the problem and providing the solution! Probably wouldn't have discovered it otherwise. I basically copied your solution into the project and it seems to work great, so it's included in the release.