-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: improve pro label #545
Conversation
666c257
to
3ca14b0
Compare
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
---|---|---|---|
🟢 | Statements | 100% | 0/0 |
🟢 | Branches | 100% | 0/0 |
🟢 | Functions | 100% | 0/0 |
🟢 | Lines | 100% | 0/0 |
Test suite run success
0 tests passing in 0 suite.
Report generated by 🧪jest coverage report action from dfecd73
Project Coverage and TestStatements : 33.65% ( 10612/31532 ) Test Suites: 58 passed, 58 total |
export interface IProLabelProps { | ||
export type IProLabelProps = { | ||
title: string; | ||
className?: string; | ||
popoverProps?: PopoverProps; | ||
colon?: boolean; | ||
size?: 'small' | 'default'; | ||
requiredMark?: boolean; | ||
} | ||
} & ( | ||
| { | ||
popoverProps?: PopoverProps; | ||
tooltipProps?: never; | ||
} | ||
| { | ||
popoverProps?: never; | ||
tooltipProps?: TooltipProps; | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est chimique le join que tu fais la, pourquoi ne pas juste rajouter le nouveau prop en optionnel dans l'interface actuelle ? De cette manière tu réduits les chances d'impacter l'existant, car l'interface est exportée.
export interface IProLabelProps {
title: string;
className?: string;
popoverProps?: PopoverProps;
tooltipProps?: TooltipProps;
colon?: boolean;
size?: 'small' | 'default';
requiredMark?: boolean;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atoulous Ca s'appel du conditional typing c'est loin d'être chimique hahah.
tooltipProps ne peut pas être défini en meme temps que popoverProps, d'où le conditional typing.
Ca n'impacte en rien l'existant.
Je peux faire ce que tu dis, mais en faisant cela, on se retrouve avec des components qui ont des props qui peuvent être définies mais qui ne font rien au final, ce qui est selon moi une mauvaise pratique.
Je peux te le mettre plus beau en séparant les types si c'est le visuel qui t'achale 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Garde comme ça si c'est ce que tu préconises 👍
Selon moi l'optional typing suffisait, mais pour le conditional je te partage une solution que je trouve plus lisible avec du naming
interface IProLabelBaseProps {
title: string;
className?: string;
colon?: boolean;
size?: 'small' | 'default';
requiredMark?: boolean;
}
interface IProLabelWithPopover extends IProLabelBaseProps {
popoverProps: PopoverProps;
tooltipProps?: never;
}
interface IProLabelWithTooltip extends IProLabelBaseProps {
tooltipProps: TooltipProps;
popoverProps?: never;
}
export type IProLabelProps = IProLabelWithPopover | IProLabelWithTooltip;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
je viens mettre mon petit grain de sel mais vous pouvez l'ignorer 😄
J'avoue que je trouve plus lisible la solution d'Aymeric mais les 2 me vont pour du conditional et in fine ça fait la même chose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sinon, voici ce que j'ai fait
interface IProLabelWithPopover {
popoverProps?: PopoverProps;
tooltipProps?: never;
}
interface IProLabelWithTooltip {
popoverProps?: never;
tooltipProps?: TooltipProps;
}
export type IProLabelProps = {
title: string;
className?: string;
colon?: boolean;
size?: 'small' | 'default';
requiredMark?: boolean;
} & (IProLabelWithPopover | IProLabelWithTooltip);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@oliviercperrier tu vas juste devoir faire un rebase et mettre a jour ta version a 10.13.0 (new features) |
c1fe7af
to
dfecd73
Compare
FEAT
Description
Make ProLabel supports Popover and Tooltip for the info icon.
[JIRA LINK]
Acceptance Criterias
Validation
Screenshot
Mention
@kstonge @luclemo