feat(PageLayout): support function type for topAlert prop #491
Conversation
arsen-afaunov
requested review from
BlackPoretsky,
DarkGenius,
Lunory and
kendoow
as code owners
|
Preview is ready. |
|
Playwright Test Component is ready. |
| {typeof topAlert === 'function' ? ( | ||
| topAlert() | ||
| ) : ( | ||
| <TopAlert className={b('top-alert')} alert={topAlert} /> |
There was a problem hiding this comment.
The base TopAlert component uses the useTopAlertHeight hook to send up information about its height, but this does not happen for the function (the --gn-top-alert-height variable remains equal to 0). Consistency of logic is violated. What can we do about it?
There was a problem hiding this comment.
Absolutely. I'll figure something out. Thank you for noticing.
| await expectScreenshot(); | ||
| }); | ||
|
|
||
| test('render story: <CustomTopAlert>', async ({mount, expectScreenshot}) => { |
There was a problem hiding this comment.
I suggest calling stories in the same style.
There was a problem hiding this comment.
Sure. I'll rename the story.
Something's wrong with the layout.
|
src/components/types.ts
Outdated
| onCloseTopAlert?: () => void; | ||
| } | ||
|
|
||
| export interface TopAlertProps extends TopAlertBaseProps { |
There was a problem hiding this comment.
The updated TopAlertProps interface still requires message, yet the component explicitly supports a render function that can render arbitrary content. Making message mandatory in the type system forces callers who only want a custom renderer to pass meaningless placeholder text to satisfy the compiler. This should be modeled as a union or by making message optional when render is provided.
| }, [opened, updateTopSize]); | ||
|
|
||
| if (!alert || !alert.message) { | ||
| return null; |
There was a problem hiding this comment.
It seems better to make message optional if the render is set.
arsen-afaunov
deleted the
support-function-type-for-PageLayout-topAlert-prop
branch
No description provided.