feat(Popover): new component #220
Conversation
NarekArshakyan
requested review from
AregSoft,
GaroGabrielyan and
hamikhambardzumyan
as code owners
NarekArshakyan
changed the title
…gene-ui-components into feature/popover-component
| /** | ||
| * Size of the popover: `xLarge`, `large`, `medium`, `small`, or `mobile`. | ||
| */ | ||
| size: "xLarge" | "large" | "medium" | "small" | "mobile"; |
There was a problem hiding this comment.
We can have default size for example medium and make the size prop optional.
package.json
Outdated
| "typings": "index.d.ts", | ||
| "scripts": { | ||
| "start": "npm run test && storybook dev -p 6006", | ||
| "start": "storybook dev -p 6006", |
There was a problem hiding this comment.
Please add run test.
| "start": "storybook dev -p 6006", | |
| "start": "npm run test && storybook dev -p 6006", |
| switch (position) { | ||
| case "top": | ||
| return { | ||
| top: currentPopoverRect.top - popoverHeight, |
There was a problem hiding this comment.
You can use destructuring to prevent repeating currentPopoverRect.
const { top, right, bottom, left } = currentPopoverRect;
| top: currentPopoverRect.top - popoverHeight, | |
| top: top - popoverHeight, | |
| left, | |
| ....... |
| </div> | ||
| {primaryButton && ( | ||
| <div className="popover__footer"> | ||
| {size !== "small" && footerContent && footerContent} |
There was a problem hiding this comment.
Why we need to repeat ?
| {size !== "small" && footerContent && footerContent} | |
| {size !== "small" && footerContent} |
|
|
||
| isOpen?: boolean; | ||
| /** | ||
| * Size of the popover: `xLarge`, `large`, `medium`, `small`, or `mobile`. |
There was a problem hiding this comment.
| * Size of the popover: `xLarge`, `large`, `medium`, `small`, or `mobile`. | |
| * Size of the popover.<br> | |
| * Possible values: `xLarge`, `large`, `medium`, `small`, or `mobile`. |
| /** | ||
| * Padding between the popover and the target element. | ||
| */ | ||
| padding: number; |
There was a problem hiding this comment.
Why padding is required for this component?
| primaryButton, | ||
| secondaryButton, | ||
| footerContent, | ||
| withArrow = true, |
There was a problem hiding this comment.
As in most cases we use Popover component withArrow: false ( DatePicker, Menu, Dropdown...) i think better to set the default value to false.
| size, | ||
| position = "bottom-center", | ||
| padding = 10, | ||
| isOpen = false, |
There was a problem hiding this comment.
We don't need the default value for isOpen prop.
| } | ||
| }); | ||
|
|
||
| if (leastOverlap > 0) { |
There was a problem hiding this comment.
'if' statement can be simplified.
| if (leastOverlap > 0) { | |
| hasOverlap = leastOverlap > 0; |
| */ | ||
|
|
||
| const Popover: FC<IPopoverProps> = ({ | ||
| size, |
There was a problem hiding this comment.
We need to add default value.
…onents into feature/popover-component
hamikhambardzumyan
left a comment
hamikhambardzumyan
left a comment
There was a problem hiding this comment.
Looks good to me
Chromatic Build Summary
Build URL: https://www.chromatic.com/build?appId=66daee7859a843dd87bfb3d7&number=301
Storybook URL: https://66daee7859a843dd87bfb3d7-fvhqkuhova.chromatic.com/