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

Refactor Switch #1309

Merged
merged 15 commits into from Oct 7, 2022
Merged

Refactor Switch #1309

merged 15 commits into from Oct 7, 2022

Conversation

aidamag
Copy link
Contributor

@aidamag aidamag commented Sep 27, 2022

Refactored some details:

  • Input has now display: none and fixed aria labels.
  • Removed animations.
  • Removed variable hasLabel.
  • We can change the state by clicking in the gap between label and switch.
  • Fixed tests for the changes of the input.

@GomezIvann GomezIvann self-requested a review September 27, 2022 10:45
@GomezIvann GomezIvann self-assigned this Sep 27, 2022
lib/src/switch/Switch.tsx Outdated Show resolved Hide resolved
agonzalez97 added 2 commits October 3, 2022 11:49
- Input has display: none
- Removed animations
- removed variable hasLabel
- We can change the state by clicking in the gap between label and switch.
@aidamag aidamag changed the title Removed useState in variable in Switch Refactor Switch Oct 3, 2022
@aidamag
Copy link
Contributor Author

aidamag commented Oct 3, 2022

Because the outline can overlap with other components, I added a margin of 12px to the sides of the switch. Chromatic tests are going to fail because of this.

Copy link
Collaborator

@GomezIvann GomezIvann left a comment

Choose a reason for hiding this comment

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

Some comments:

  • I see a lot this piece of code:
if (typeof onChange === "function") {
   onChange(isChecked);
}

We now do not have the need of checking the type since typescript does that for us. We can refactor this code by simply: onChange?.(isChecked);. What do you think?

  • In the onChange handler handlerSwitchChange we use the event.target.value, which can only be true or false, why do we check it with ?? operator? Also, the expression checked === undefined only check undefined value, but not null (which is very common). My suggest here is:
const handlerSwitchChange = (event) => {
   checked ?? setInnerChecked(event.target.checked);
   onChange?.(checked ?? event.target.checked);
};

lib/src/switch/Switch.tsx Outdated Show resolved Hide resolved
lib/src/switch/Switch.tsx Outdated Show resolved Hide resolved
@aidamag
Copy link
Contributor Author

aidamag commented Oct 5, 2022

  • Removed type checking for onChange function.
  • About the handlerSwitchChange, we need to check innerChecked for the uncontrolled. With your solution, uncontrolled doesn't work. Removed event.target.checked because it was always undefined.

Edited
I had to put again the event.target.checked in handlerSwitchChange because it is needed for the uncontrolled too.

@GomezIvann
Copy link
Collaborator

  • Removed type checking for onChange function.
  • About the handlerSwitchChange, we need to check innerChecked for the uncontrolled. With your solution, uncontrolled doesn't work. Removed event.target.checked because it was always undefined.

Yes, you are right. My solution was for a handler in a <input type="checkbox" />, and in this case, it is on a <div>, which of course doesn't have a checked property.

lib/src/switch/Switch.tsx Outdated Show resolved Hide resolved
lib/src/switch/Switch.tsx Outdated Show resolved Hide resolved
lib/src/switch/Switch.tsx Outdated Show resolved Hide resolved
lib/src/switch/Switch.tsx Outdated Show resolved Hide resolved
@GomezIvann GomezIvann merged commit e922b5b into master Oct 7, 2022
@GomezIvann GomezIvann deleted the aida-switch branch October 7, 2022 09:33
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.

None yet

2 participants