Checkbox component #27

Hasmik8 · 2020-10-19T10:36:35Z

Description
I've created checkbox component, which has two props: checked and disabled, which are boolean parameteres.

View

S-ayanide · 2020-10-20T05:47:21Z

Can you please resolve the DeepScan issues and also update your branch to the latest which has Kubera Portal and Kuber Core themes too. Also, update your stories accordingly.

AVRahul · 2020-10-21T06:14:32Z

Based on the displayed screenshots, In the mentioned issue #11 there were more states present like Inactive, Active, hover, etc. Please check the Figma design once.

src/core/CheckBox/base.ts

src/core/CheckBox/CheckBox/styles.ts

src/core/CheckBox/CheckBox/__tests__/CheckBox.test.tsx

Signed-off-by: Hasmik8 <[email protected]>

codesniffy · 2020-10-22T11:46:20Z

package.json has been modified

arkajyotiMukherjee · 2020-10-23T10:28:09Z

src/core/CheckBox/__tests__/CheckBox.test.tsx

+    );
+    let checkbox = getByTestId('checkbox') as HTMLInputElement;
+    fireEvent.click(checkbox);
+    expect(checkbox.checked).toBeFalsy();


how come if the initial value is false....after clicking it becomes false again (toBeFalsy)
Can you explain the test cases in the comments on what is being done...
I might have misunderstood it too...apologies for that...if that's the case...but regardless explanation for test cases is good

Signed-off-by: Hasmik8 <[email protected]>

codesniffy · 2020-10-23T14:25:12Z

package.json has been modified

Signed-off-by: Hasmik8 <[email protected]>

codesniffy · 2020-10-26T14:45:47Z

package.json has been modified

S-ayanide · 2020-10-27T11:17:20Z

src/core/CheckBox/CheckBox.tsx

@@ -0,0 +1,28 @@
+import { Checkbox } from '@material-ui/core';
+import React, { useState } from 'react';
+// import clsx from 'clsx';


Can you please remove commented lines!

S-ayanide · 2020-10-27T11:18:24Z

src/core/CheckBox/CheckBox.tsx

+interface CheckBoxProps extends BaseCheckboxProps {
+  checked: boolean;
+}


Checkbox API already provides checked thus there is no requirement of creating it again. The API

S-ayanide · 2020-10-27T11:18:48Z

src/core/CheckBox/CheckBox.tsx

+  checked: boolean;
+}
+
+const CheckBox: React.FC<CheckBoxProps> = ({ disabled, checked }) => {


Suggested change

const CheckBox: React.FC<CheckBoxProps> = ({ disabled, checked }) => {

const CheckBox: React.FC = ({ disabled, checked }) => {

S-ayanide · 2020-10-27T11:19:50Z

src/core/CheckBox/base.ts

+import { CheckboxProps } from '@material-ui/core/Checkbox';
+
+export type BaseCheckboxProps = Omit<CheckboxProps, 'checked'>;


Here checked is Omitted but again declared as a type in checkbox.tsx. If we need checked there is no need to Omit it.

This base.ts file is not required.

S-ayanide · 2020-10-27T11:23:46Z

src/core/CheckBox/CheckBox.tsx

+      checkedIcon={<CheckOutlinedIcon />}
+      disabled={disabled}
+      checked={check}
+      icon={<span></span>}


This is not required. It's an empty span.

@S-ayanide This icon appears when checkbox is unchecked and when I delete that icon design breaks,this empty span is required

I think there is a way around. if you take a look at the third checkbox from Basic Checkbox here you'll find they don't use icons as a parameter at all still you can check and uncheck without breaking the design.

Can we have a workaround for this icon parameter! Did the above reference help?

Signed-off-by: Hasmik8 <[email protected]>

S-ayanide · 2020-10-29T04:34:22Z

src/core/CheckBox/CheckBox.tsx

+      checkedIcon={<CheckOutlinedIcon />}
+      disabled={disabled}
+      checked={check}
+      icon={<span></span>}


Can we have a workaround for this icon parameter! Did the above reference help?

Hasmik8 · 2020-10-29T05:10:34Z

@S-ayanide
icon parameter is empty, I can add CSS code for it to work.

Signed-off-by: Hasmik8 <[email protected]>

S-ayanide · 2020-11-02T13:38:08Z

src/core/CheckBox/CheckBox.tsx

+      checkedIcon={<CheckOutlinedIcon />}
+      disabled={disabled}
+      checked={check}
+      onChange={() => setChecked(!check)}


With this change if I pass Checkbox check={true} and mutate the state here then checkbox would be unchecked but the place where we have implemented it still would remain true which is a loophole

@S-ayanide When passing the checkox checked paraemeter=true via props, then checkbox is being checked and when clicking on the checkbox, it becomes unchecked. And vica versa. If there is an issue, let me know pls

Hasmik8 requested review from arkajyotiMukherjee, AVRahul and S-ayanide as code owners October 19, 2020 10:36

S-ayanide assigned Hasmik8 Oct 20, 2020

S-ayanide linked an issue Oct 20, 2020 that may be closed by this pull request

CC: Checkbox #11

Open

S-ayanide added the adding-component label Oct 20, 2020

Hasmik8 force-pushed the checkbox-component branch from c2ccb79 to d7301c9 Compare October 20, 2020 13:49

checkbox-component

f8206c9

Hasmik8 force-pushed the checkbox-component branch from d7301c9 to f8206c9 Compare October 20, 2020 13:53

Hasmik8 added the enhancement New feature or request label Oct 21, 2020

arkajyotiMukherjee suggested changes Oct 22, 2020

View reviewed changes

src/core/CheckBox/base.ts Outdated Show resolved Hide resolved

src/core/CheckBox/CheckBox/styles.ts Outdated Show resolved Hide resolved

src/core/CheckBox/CheckBox/__tests__/CheckBox.test.tsx Outdated Show resolved Hide resolved

codesniffy bot added the PR: Changes Requested label Oct 22, 2020

fix issues

f3ee9c7

Signed-off-by: Hasmik8 <[email protected]>

codesniffy bot added the size/L label Oct 22, 2020

Hasmik8 removed the PR: Changes Requested label Oct 22, 2020

arkajyotiMukherjee suggested changes Oct 23, 2020

View reviewed changes

codesniffy bot added the PR: Changes Requested label Oct 23, 2020

create test checkbox

4567210

Signed-off-by: Hasmik8 <[email protected]>

Hasmik8 removed the PR: Changes Requested label Oct 23, 2020

Hasmik8 added 2 commits October 26, 2020 18:37

fix error test

2d13d60

Signed-off-by: Hasmik8 <[email protected]>

fix

95fe63f

Signed-off-by: Hasmik8 <[email protected]>

S-ayanide suggested changes Oct 27, 2020

View reviewed changes

codesniffy bot added the PR: Changes Requested label Oct 27, 2020

Hasmik8 added 2 commits October 28, 2020 16:53

fix interface && base props issue

bcda10f

Signed-off-by: Hasmik8 <[email protected]>

update branch

f0e7d2e

Signed-off-by: Hasmik8 <[email protected]>

Hasmik8 removed the PR: Changes Requested label Oct 28, 2020

S-ayanide suggested changes Oct 29, 2020

View reviewed changes

codesniffy bot added the PR: Changes Requested label Oct 29, 2020

remove icon

bb6f381

Signed-off-by: Hasmik8 <[email protected]>

Hasmik8 added PR: Review Requested and removed PR: Changes Requested labels Oct 29, 2020

S-ayanide suggested changes Nov 2, 2020

View reviewed changes

codesniffy bot added the PR: Changes Requested label Nov 2, 2020

arkajyotiMukherjee closed this Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checkbox component #27

Checkbox component #27

Hasmik8 commented Oct 19, 2020

S-ayanide commented Oct 20, 2020

AVRahul commented Oct 21, 2020

codesniffy bot commented Oct 22, 2020

arkajyotiMukherjee Oct 23, 2020

codesniffy bot commented Oct 23, 2020

codesniffy bot commented Oct 26, 2020

S-ayanide Oct 27, 2020

S-ayanide Oct 27, 2020

S-ayanide Oct 27, 2020

S-ayanide Oct 27, 2020

S-ayanide Oct 27, 2020

Hasmik8 Oct 28, 2020

S-ayanide Oct 28, 2020

S-ayanide Oct 29, 2020

S-ayanide Oct 29, 2020

Hasmik8 commented Oct 29, 2020

S-ayanide Nov 2, 2020

Hasmik8 Nov 4, 2020

	const CheckBox: React.FC<CheckBoxProps> = ({ disabled, checked }) => {
	const CheckBox: React.FC = ({ disabled, checked }) => {

		import { CheckboxProps } from '@material-ui/core/Checkbox';

		export type BaseCheckboxProps = Omit<CheckboxProps, 'checked'>;

Checkbox component #27

Checkbox component #27

Conversation

Hasmik8 commented Oct 19, 2020

S-ayanide commented Oct 20, 2020

AVRahul commented Oct 21, 2020

codesniffy bot commented Oct 22, 2020

Choose a reason for hiding this comment

codesniffy bot commented Oct 23, 2020

codesniffy bot commented Oct 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hasmik8 commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment