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

Radiobutton #911

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@aSquare14
Member

aSquare14 commented Jul 4, 2018

  • This PR resolves #779.
  • So, as discussed with @nshki I split this component into two parts. One is radio-button and other is checkbox.
  • The CSS files are almost same for both. I decided to make radiobuttons circular as that is the convention.
  • I have filled out colour in CSS based on how it looks in Figma rather than the CSS that is generated.

Here are the screenshots.
screenshot from 2018-07-04 22-47-32
screenshot from 2018-07-04 22-47-38

@aSquare14 aSquare14 requested review from nshki and julianguyen Jul 4, 2018

@aSquare14 aSquare14 force-pushed the aSquare14:radiobutton branch from 02abb59 to aeaedf6 Jul 5, 2018

@julianguyen

Looking good so far! Great work 👍

Let me know if you have any questions about my comments.

In addition to the comments, I highly encourage you to write tests for the Checkbox and Radiobutton components i.e. testing toggling states.

It also looks like there are some listing errors as indicated here

export default class Checkbox extends React.Component<Props, State>{
constructor(props: Props) {
super(props);
this.state = { checkboxState: true};

This comment was marked as resolved.

@julianguyen

julianguyen Jul 5, 2018

Member

Add space between true and }

render() {
const checkedOrNot = [];
checkedOrNot.push(
<p>{this.state.checkboxState ? 'Unchecked' : 'Checked'}</p>

This comment has been minimized.

@julianguyen

julianguyen Jul 5, 2018

Member

Unchecked and Checked should be i18n keys

This comment has been minimized.

@aSquare14

aSquare14 Jul 6, 2018

Member

I don't know how to do this. Can you help me figure it out .

<input type="checkbox" tabIndex="1" onChange={this.toggle}/>
<div className={css.check}></div>
</label>
<br/>

This comment was marked as resolved.

@julianguyen

julianguyen Jul 5, 2018

Member

Instead of line breaks, we should explicitly set a px value for the spacing between these two labels.

.check {
position: absolute;
cursor: pointer;
top: 0; left: 0; right: 0; bottom: 0;

This comment was marked as resolved.

@julianguyen

julianguyen Jul 5, 2018

Member

I'd put these in separate lines.

<input type="radio" value="option1" checked={this.state.value === 'option1'} onChange={this.onChange} />
<div className={css.check}>{}</div>
</label>
<br />

This comment was marked as resolved.

@julianguyen

julianguyen Jul 5, 2018

Member

Instead of line breaks, we should explicitly set a px value for the spacing between these two labels.

@aSquare14 aSquare14 force-pushed the aSquare14:radiobutton branch from 715be98 to 8ae97ee Jul 6, 2018

};
type State = {
checkboxState: boolean;

This comment has been minimized.

@julianguyen

julianguyen Jul 7, 2018

Member

I would rename this to checked so it's more explicit what this state is referring to

export default class Checkbox extends React.Component<Props, State> {
constructor(props: Props) {
super(props);
this.state = { checkboxState: true };

This comment has been minimized.

@julianguyen

julianguyen Jul 7, 2018

Member

When you rename this to checked, then the state would be false here.

<input type="checkbox" tabIndex="0" onChange={this.toggle} />
<div className={css.check}>{}</div>
</label>
{/* <label htmlFor={id} className={css.switch}>Option2

This comment has been minimized.

@julianguyen

julianguyen Jul 7, 2018

Member

Why is this commented out?

@aSquare14

This comment has been minimized.

Member

aSquare14 commented Jul 10, 2018

@julianguyen I am working on Ruby presently. Wanted a break from ReactJs. Shall write Radiobutton tests in a few days ! 😄

@julianguyen

This comment has been minimized.

Member

julianguyen commented Jul 10, 2018

No problem at all! It's always nice to switch languages :P When this PR is ready for review, just let me know and I'll take a look.

@aSquare14

This comment has been minimized.

Member

aSquare14 commented Jul 13, 2018

@julianguyen Ready for review !

@julianguyen julianguyen self-requested a review Jul 16, 2018

@julianguyen

Good progress so far :D

There seems to be a misunderstanding about how to build out the component. So basically we want to pass in the label and input field value as props to the component.

Let me know if you want to pair on this.

After we clean this up, let's pair together on unit tests :)

export default class Checkbox 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 16, 2018

Member

Since this.props.check is a mandatory prop, then we could just pass in this.props.checked without || false

}
render() {
const checkedOrNot = [];

This comment has been minimized.

@julianguyen

julianguyen Jul 16, 2018

Member

Where is checkedOrNot being used? It's been initialized and set to values, but I don't see it being used anywhere.

return (
<span>
<form>
<label htmlFor={id} className={css.switch} >Option1

This comment has been minimized.

@julianguyen

julianguyen Jul 16, 2018

Member

Do we always want this component to have Option1 and Option2 as labels?

render() {
const { id } = this.props;
return (
<div>

This comment has been minimized.

@julianguyen

julianguyen Jul 16, 2018

Member

We can get rid of the parent level div since there's a parent form element.

return (
<div>
<form>
<label htmlFor={id} className={css.switch}> Option1

This comment has been minimized.

@julianguyen

julianguyen Jul 16, 2018

Member

We should be passing in props for the Option1 text since we do not always want our component to be Option1

<div>
<form>
<label htmlFor={id} className={css.switch}> Option1
<input type="radio" value="option1" checked={this.state.value === 'option1'} onChange={this.onChange} />

This comment has been minimized.

@julianguyen

julianguyen Jul 16, 2018

Member

Same here for the value

<input type="radio" value="option1" checked={this.state.value === 'option1'} onChange={this.onChange} />
<div className={css.check}>{}</div>
</label>
<label htmlFor={id} className={css.switch}>Option2

This comment has been minimized.

@julianguyen

julianguyen Jul 16, 2018

Member

Let's get rid of this second radio button, since the component should just be one radio button. If we want multiple on a page then we will do <RadioButton etc. /> multiple times.

<input type="checkbox" tabIndex="0" onChange={this.toggle} />
<div className={css.check}>{}</div>
</label>
<label htmlFor={id} className={css.switch}>Option2

This comment has been minimized.

@julianguyen

julianguyen Jul 16, 2018

Member

Let's get rid of this second checkbox, since the component should just be one radio button. If we want multiple on a page then we will do <Checkbox etc. /> multiple times.

@julianguyen

This comment has been minimized.

Member

julianguyen commented Jul 18, 2018

Some updates:
@aSquare14 and I paired on fixing up the Checkbox component. Based on that, she'll clean up the RadioButton component :)

We're going to pair after on creating CheckboxGroup and RadioButtonGroup components.

@julianguyen julianguyen force-pushed the aSquare14:radiobutton branch from 8e072ad to dab7a44 Jul 29, 2018

New review needed !

@julianguyen julianguyen added the wip label Aug 2, 2018

@julianguyen julianguyen force-pushed the aSquare14:radiobutton branch from dd07d3a to 94ecb19 Aug 4, 2018

@julianguyen julianguyen force-pushed the aSquare14:radiobutton branch from 94ecb19 to 85afc87 Aug 4, 2018

@julianguyen julianguyen referenced this pull request Aug 4, 2018

Merged

Checkbox component #930

@aSquare14 aSquare14 added the RGSoC 2018 label Sep 4, 2018

@aSquare14

This comment has been minimized.

Member

aSquare14 commented Nov 6, 2018

@julianguyen Should I close up this PR ?

@julianguyen

This comment has been minimized.

Member

julianguyen commented Nov 6, 2018

@aSquare14 It's up to you! If you want to finish it up, feel free to keep it open. Otherwise closing it is fine. We actually don't use any radio buttons in the app at the moment, but it doesn't mean we shouldn't create this component.

@aSquare14

This comment has been minimized.

Member

aSquare14 commented Nov 6, 2018

I'll close it. I learnt a lot making this component and I'm happy to have worked on it ! :)

@aSquare14 aSquare14 closed this Nov 6, 2018

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