Skip to content
This repository was archived by the owner on Oct 19, 2023. It is now read-only.

Conversation

@tinhvqbk
Copy link
Contributor

  • Add type definitions for this package in src/index.d.ts for the current version 2.0.3
  • Update scripts/build.js, .eslintignore

@tinhvqbk tinhvqbk requested review from namdaoduy and nvh95 April 24, 2022 12:06
@tinhvqbk tinhvqbk changed the base branch from master to release/2.0.4 April 27, 2022 09:56
@namdaoduy
Copy link
Contributor

Pls update CHANGELOG.md @tinhvqbk

@tinhvqbk
Copy link
Contributor Author

Pls update CHANGELOG.md @tinhvqbk

Updated

@tinhvqbk tinhvqbk requested a review from thangtvse April 28, 2022 11:27
Copy link
Contributor

@namdaoduy namdaoduy left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot, @tinhvqbk 🚀

@tinhvqbk tinhvqbk requested a review from namdaoduy April 29, 2022 10:02
src/index.d.ts Outdated
| 'information_subtle'
| 'negative'
| 'negative_subtle';
textClassName: string | string[];

Choose a reason for hiding this comment

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

This prop should be optional

src/index.d.ts Outdated

export interface BubbleChatProps extends BasicProps {
isTyping?: boolean;
text?: string;

Choose a reason for hiding this comment

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

text can receive html element too, like the embed html in study_webapp ExpertMessage.tsx

src/index.d.ts Outdated
| 'transparentLight';
avatar?: string | FuncType;
time?: string | FuncType;
options?: OptionType;

Choose a reason for hiding this comment

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

OptionType[]

src/index.d.ts Outdated
Comment on lines 288 to 290
textClassName?: string | string[];
actionBar?: React.ReactNode;
actionBarClassName?: string | string[];

Choose a reason for hiding this comment

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

Why textClassName and actionBarClassName can receive array of string in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, should remove string[] in type

src/index.d.ts Outdated
disabled?: boolean;
nonUppercase?: boolean;
onlyIcon?: boolean;
textClassName?: string | string[];

Choose a reason for hiding this comment

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

Same question above

Comment on lines +479 to +493
export interface FileAttachmentProps extends BasicProps {
fileType?:
| 'undefined'
| 'text'
| 'image'
| 'code'
| 'spreadsheet'
| 'query'
| 'powerpoint';
fileTypeLabel?: string | FuncType;
show?: boolean;
onClose?: () => void;
closeButton?: boolean;
actionLeft?: FuncType;
actionRight?: FuncType;

Choose a reason for hiding this comment

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

Miss filename

as?: React.ElementType;
}
export interface FormFeedbackProps extends BasicProps {
type: 'valid' | 'invalid';

Choose a reason for hiding this comment

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

Optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should required to more clearly what is Feedback type

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@tinhvqbk tinhvqbk May 4, 2022

Choose a reason for hiding this comment

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

I think it bug on original source code. The prop type made FormFeedback more clearly.
Ex:

<Form.Feedback>Message</Form.Feedback>
<Form.Feedback type="valid"></Form.Feedback>

What is different? Which code is clean and easy to understand?

Choose a reason for hiding this comment

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

Fare point, just to make sure there is no place using this Form.Feedback without passing type prop in all projects

src/index.d.ts Outdated
export interface FormLabelProps extends BasicProps {
sizeLabel?: InputSize;
required?: boolean;
htmlFor: string;

Choose a reason for hiding this comment

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

Optional

src/index.d.ts Outdated
export interface FormInputProps extends BasicProps {
type?: string;
value?: string | number;
id: string;

Choose a reason for hiding this comment

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

Optional

src/index.d.ts Outdated
}
export interface FormSelectProps extends BasicProps {
value?: string | number;
id: string;

Choose a reason for hiding this comment

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

Optional

@tinhvqbk tinhvqbk requested a review from namdaoduy May 4, 2022 11:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants