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 component added #910

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@prateksha
Member

prateksha commented Jul 3, 2018

#780 Resolved!

@prateksha prateksha requested a review from nshki Jul 3, 2018

@prateksha prateksha requested a review from julianguyen Jul 3, 2018

aSquare14 and others added some commits Jul 3, 2018

@nshki

Overall this looks great, thanks a bunch for working on this! :)

I left a few minor comments. Would love to see what @julianguyen thinks as well. We may need a way to provide a name attribute to the hidden input in this component for forms.

const sliderClassNames = `${css.slider}`;
return (
<div>
<label className = {switchClassNames}>

This comment has been minimized.

@nshki

nshki Jul 3, 2018

Member

Let's get rid of the extra spaces around the = here.

<div>
<label className = {switchClassNames}>
<input type="checkbox" onChange={this.handleChange}/>
<div className = {sliderClassNames}></div>

This comment has been minimized.

@nshki

nshki Jul 3, 2018

Member

Let's get rid of the extra spaces around the = here.

}
handleChange = () => {
this.setState({ checked: !this.state.checked })

This comment has been minimized.

@nshki

nshki Jul 3, 2018

Member

Let's add the missing ;!

return (
<div>
<label className = {switchClassNames}>
<input type="checkbox" onChange={this.handleChange}/>

This comment has been minimized.

@nshki

nshki Jul 3, 2018

Member

Let's add a space before the />.

position: absolute;
top: -99999px;
left: -99999px;
}

This comment has been minimized.

@nshki

nshki Jul 3, 2018

Member

Ahh, while this works, I think you can get away with just using display: none;!

background-color: $mulberry-wood;
border: 0px solid rgba(255, 255, 255, 0.7);
border-radius: 5px;
-webkit-transition: .2s;

This comment has been minimized.

@nshki

nshki Jul 3, 2018

Member

You can omit the vendor prefix!

background-color: rgba(255, 255, 255, 0.7);
border: 0px solid rgba(255, 255, 255, 0.7);
border-radius: 5px;
-webkit-transition: .2s;

This comment has been minimized.

@nshki

nshki Jul 3, 2018

Member

You can omit the vendor prefix here!

@julianguyen

Great work @prateksha! 🎉 Looking fantastic so far. I left some comments you should review, let me know if you have any questions. +1 to all of @nshki's suggestions too :D

export default class Switch extends React.Component<Props, State> {
constructor(props: Props) {
super(props);
this.state = { checked: this.props.checked || false };

This comment has been minimized.

@julianguyen

julianguyen Jul 3, 2018

Member

You could do !!this.props.checked instead

This comment has been minimized.

@prateksha

prateksha Jul 5, 2018

Member

In line 16, why is it 'this.props.checked' and not 'this.state.checked'?

This comment has been minimized.

@julianguyen

julianguyen Jul 7, 2018

Member

Ooh, you're right it should be !!this.state.checked

};
render () {
const switchClassNames = `${css.switch}`;

This comment has been minimized.

@julianguyen

julianguyen Jul 3, 2018

Member

I don't think you need the back ticks here or in the following line. You should be able to pass in the values directly.

}
input:checked + .slider {
background-color: #BACF88;

This comment has been minimized.

@julianguyen

julianguyen Jul 3, 2018

Member

Let's put this value in a variable! Same goes for value on line 42.

.slider:after {
position: absolute;
content: "OFF";

This comment has been minimized.

@julianguyen

julianguyen Jul 3, 2018

Member

Let's pass in an i18n key here

This comment has been minimized.

@prateksha

prateksha Jul 5, 2018

Member

@julianguyen How do I pass an i18n key here? I'm unable to understand.

}
input:checked + .slider:after {
content: 'ON';

This comment has been minimized.

@julianguyen

julianguyen Jul 3, 2018

Member

Let's pass in an i18n key here

This comment has been minimized.

@prateksha

prateksha Jul 10, 2018

Member

@julianguyen How do I pass an i18n key here? I'm unable to understand.

checked: boolean
};
export default class Switch extends React.Component<Props, State> {

This comment has been minimized.

@julianguyen

julianguyen Jul 3, 2018

Member

It would be great to create a unit test for this component and test toggling it.

This comment has been minimized.

@aSquare14

aSquare14 Jul 5, 2018

Member

I've tested this ! Let me know if it is the right way !

aSquare14 and others added some commits Jul 5, 2018

@julianguyen

Thanks for adding test coverage!

Left some comments, let me know if you have any questions.

Looking great so far :)

export default class Switch extends React.Component<Props, State> {
constructor(props: Props) {
super(props);
this.state = { checked: this.props.checked || false };

This comment has been minimized.

@julianguyen

julianguyen Jul 7, 2018

Member

Ooh, you're right it should be !!this.state.checked

let wrapper = shallow(<Switch />);
expect(wrapper.find('input').exists()).toEqual(true);
wrapper.find('input').simulate('click');
expect(wrapper.find('input').exists()).toEqual(true);

This comment has been minimized.

@julianguyen

julianguyen Jul 7, 2018

Member

I would test the value of checked i.e. wrapper.state('checked')

aSquare14 and others added some commits Jul 10, 2018

@aSquare14

This comment has been minimized.

Member

aSquare14 commented Sep 15, 2018

I think rebase your branch from master before proceeding. Your error in storybook should be fixed.

@julianguyen

This comment has been minimized.

Member

julianguyen commented Oct 6, 2018

I'm going to finish up this feature. I'm sorry we didn't get a chance to wrap this up for RGSoC.

@julianguyen julianguyen closed this Oct 6, 2018

@aSquare14

This comment has been minimized.

Member

aSquare14 commented Oct 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment