[Review] Kiet Nguyen - React Practice #1

[kietnguyen-agilityio]

• README - Incorrect App Bio.
• Storybook - I get this error while running Storybook, has to use this workaround to run successfully. Maybe this should be included in issues of README.
• FeatureCard - should only extend part of Image props, e.g. it does not need image size prop
• Form - is it need to have classes prop if there is only one form
• Form - should define action to set value for text fields in Form, currently can not input value to text fields
• Image - Can not see different between statusImage:primary-image and statusImage:secondary-image, storybook should show this
• Image - Not sure what is index (use for data-index) prop for, please refer to see what data-* attributes means
• Input - it's text input, why does it need type prop? if you have checkbox, radio button, etc, they should be other components
• List - should prop types extends from List Item's?
• ListItem - should only extend part of Image props?
• Select - should have onChange action?
• Select - can use HTML select tag?
• TextArea - should have onChange action?
• Storybook - No need stories for sections and pages, please research more about purpose of Storybook
• Code style and performance: all props can be spread in arguments of components
• Code structure: some define interfaces in interface folder, some defined directly in components, make sure the define location is consistent and has a purpose

[LanHuong142002]

Select - should have onChange action?
Select - can use HTML select tag?

Because I customed the select option to make it the same as with the design. But I can't customize all the select options as I want so I try to create another selection by myself. And use onClick instead onChange because the selection which I customed just a div or span

[LanHuong142002]

Storybook - I get this error while running Storybook, has to use this workaround to run successfully. Maybe this should be included in issues of README.

About this, I have a small note at the end of README.md. Do you mean that the note is not working and needs to be added public-hoist-pattern[]=*storybook*?

[kietnguyen-agilityio]

Select - should have onChange action?
Select - can use HTML select tag?

Because I customed the select option to make it the same as with the design. But I can't customize all the select options as I want so I try to create another selection by myself. And use onClick instead onChange because the selection which I customed just a div or span

I see, but if another component uses Select, can it take Select's value when the value changes? I don't see any action to take this value.

[kietnguyen-agilityio]

Storybook - I get this error while running Storybook, has to use this workaround to run successfully. Maybe this should be included in issues of README.

About this, I have a small note at the end of README.md. Do you mean that the note is not working and needs to be added public-hoist-pattern[]=*storybook*?

My bad. I see the note now. But it needs a bit update, the .npmrc is already existed, usually at home (~), no need to create.

[LanHuong142002]

Select - should have onChange action?
Select - can use HTML select tag?

Because I customed the select option to make it the same as with the design. But I can't customize all the select options as I want so I try to create another selection by myself. And use onClick instead onChange because the selection which I customed just a div or span

I see, but if another component uses Select, can it take Select's value when the value changes? I don't see any action to take this value.

I have a callback function onClick to do those things. Pass function set state from Form component to Select Component and Select Item component and get it with e.target. You can see it more clearly in Form component

[LanHuong142002]

Code style and performance: all props can be spread in arguments of components

• You mean this?

const Textarea = (props: Props) => {
  return (
    <textarea
      className='form-input form-textarea'
      placeholder={props.placeholder}
      value={props.value}
      {...props}
    ></textarea>
  );
};

to

const Textarea = (props: Props) => {
  return <textarea className='form-input form-textarea' {...props}></textarea>;
};

• If it is right, it just only is used for components that have many different attributes on the same tag, isn't it?

[kietnguyen-agilityio]

Code style and performance: all props can be spread in arguments of components

• You mean this?

const Textarea = (props: Props) => {
  return (
    <textarea
      className='form-input form-textarea'
      placeholder={props.placeholder}
      value={props.value}
      {...props}
    ></textarea>
  );
};

to

const Textarea = (props: Props) => {
  return <textarea className='form-input form-textarea' {...props}></textarea>;
};

• If it is right, it just only is used for components that have many different attributes on the same tag, isn't it?

It can be like this

const Textarea = ({placeholder, value} : Props) => {
  return (
    <textarea
      className='form-input form-textarea'
      placeholder={placeholder}
      value={value}
    />
  );
};

similar for other props