Feature/accordion by WenSuskey · Pull Request #173 · patricklindev/pat-ui

WenSuskey · 2023-07-05T17:35:59Z

Accordion component created

PHANTOMS0308 · 2023-07-07T04:41:30Z

src/components/Accordion/Accordion.tsx

@@ -0,0 +1,158 @@
+import React, { FC, useState } from 'react';
+import { action } from '@storybook/addon-actions';


This line shouldn't be here, this is storybook things

PHANTOMS0308 · 2023-07-07T04:42:05Z

src/components/Accordion/Accordion.tsx

@@ -0,0 +1,158 @@
+import React, { FC, useState } from 'react';
+import { action } from '@storybook/addon-actions';
+import up from '../../asset/icon/expanded-up.svg';


It would be better if you name it as upIcon or upSVG, same for the next line.

PHANTOMS0308 · 2023-07-07T04:44:56Z

src/components/Accordion/Accordion.tsx

+import up from '../../asset/icon/expanded-up.svg';
+import down from '../../asset/icon/expanded-down.svg';
+import { classNames } from '../../utils/classNames';
+export type AccordionType = 'default' | 'checkbox';


If you don't actually need this AccordionType type anywhere else, you could simply write it inside the IAccordionProps interface

PHANTOMS0308 · 2023-07-07T04:47:13Z

src/components/Accordion/Accordion.tsx

+  /** set Accordion expanded */
+  expanded?: boolean;
+  /** set action on title clicked */
+  onChange?: () => void;


This definition is really not strict. Since you are using TS, you would like to define a more detailed onChange function type

PHANTOMS0308 · 2023-07-07T04:49:00Z

src/components/Accordion/Accordion.tsx

+  /**set Accordion type */
+  type?: AccordionType;
+}
+export type patAccordionProps = IAccordionProps;


Don't quite understand what is the purpose of this line?

PHANTOMS0308 · 2023-07-07T04:55:37Z

src/components/Accordion/Accordion.tsx

+    ...rest
+  } = props;
+  let initialState: boolean = expanded === undefined ? false : expanded;
+  const [show, setShow] = useState<boolean | false>(initialState);


You can delete line 39, and swap line 40 with

const [show, setShow] = useState<boolean>(initialState ?? false);

PHANTOMS0308 · 2023-07-07T05:11:52Z

src/components/Accordion/Accordion.tsx

+  const handleDefaultClick = () => {
+    setShow(!show);
+  };
+  const isExpanded: string = show ? 'isExpanded' : '';


Honestly, the variable name is hinting a boolean, but instead is a string. I kinda understand you wanna have some shortcut for css classname later, but this is quite contradictory naming and data type!=

PHANTOMS0308 · 2023-07-07T05:13:08Z

src/components/Accordion/Accordion.tsx

+  };
+  const isExpanded: string = show ? 'isExpanded' : '';
+  const isDisabled: string = disabled ? 'isDisabled' : '';
+  const isControlExpanded: string = props.expanded ? 'isExpanded' : '';


No need for props.expanded since you already destructured that.

PHANTOMS0308 · 2023-07-07T05:20:45Z

src/components/Accordion/Accordion.tsx

+  }
+
+  let accordion;
+  if (props.onChange === undefined) {


I believe you could simply just use a conditional statement inside the onClick, for example

onClick={props.onChange ? props.onChange : handleDefaultClick}

so that you dont need to have two long code block

PHANTOMS0308 · 2023-07-07T05:23:56Z

src/components/Accordion/Accordion.tsx

+          </button>
+        </div>
+
+        {show ? (


Same here

<p className={styleClasses + ' accordion-description' + show ? '' : 'hiden'}> {props.children} </p>

PHANTOMS0308 · 2023-07-07T05:25:16Z

src/components/Accordion/_Accordion.scss

@@ -0,0 +1,94 @@
+


I would say using nesting will increase readability for Sass user

PHANTOMS0308 · 2023-07-07T05:26:42Z

src/components/Accordion/Accordion.tsx

+      </div>
+    );
+  }
+  return accordion;


So now you can simply just return the JSX, instead of extract the logic into a variable with if statement. Also I realized that you used a lot of props.xxx, but you actually de-structure most of them at the beginning of your component. It seems redundant to me.

PHANTOMS0308

Your component is working great, the tests are good. However, you could spend some extra time to refactor your code, more specifically to remove redundant logics.

zeke79 and others added 9 commits June 27, 2023 16:25

new branch

53002a1

finish goal 1 & 2

1e53287

npm

0a6953c

fix string

f6b5aad

add some css

0f9d1e8

80% finish task 1 & 2

420ac3e

fix scss

a101829

finished scss, just need unit test

9ca3a9d

finish accordion

97d4514

PHANTOMS0308 reviewed Jul 7, 2023

View reviewed changes

src/components/Accordion/_Accordion.scss

@@ -0,0 +1,94 @@

Copy link
Copy Markdown

PHANTOMS0308 Jul 7, 2023

Choose a reason for hiding this comment

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

I would say using nesting will increase readability for Sass user

PHANTOMS0308 reviewed Jul 7, 2023

View reviewed changes

PHANTOMS0308 approved these changes Jul 7, 2023

View reviewed changes

		@@ -0,0 +1,158 @@
		import React, { FC, useState } from 'react';
		import { action } from '@storybook/addon-actions';

Conversation

WenSuskey commented Jul 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PHANTOMS0308 Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PHANTOMS0308 Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PHANTOMS0308 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PHANTOMS0308 Jul 7, 2023 •

edited

Loading

PHANTOMS0308 Jul 7, 2023 •

edited

Loading