[UIK-4127][feedback-form] added illustrationElement/NoticeComponent props #2659
Conversation
|
|
||
| class FeedbackRatingRoot extends Component< | ||
| FeedbackRatingProps, | ||
| FeedbackRatingProps & { NoticeComponent: typeof FeedbackRatingRoot.defaultProps['NoticeComponent'] }, |
There was a problem hiding this comment.
I think, you shouldn't set type here if you have NoticeComponent in the FeedbackRatingProps
There was a problem hiding this comment.
updated
| /** Illustration element */ | ||
| illustrationElement?: React.ReactElement; | ||
| /** Notice smart component like NoticeSmart/NoticeFH */ | ||
| NoticeComponent?: React.ComponentType<NoticeSmartProps>; |
There was a problem hiding this comment.
what do you think about just Illustration and Notice - both from upper case and with specifics in type?
There was a problem hiding this comment.
P.S.
Illustration?: Intergalactic.Component<'svg', IllustrationProps>;
Notice?: typeof Notice;
There was a problem hiding this comment.
updated
There was a problem hiding this comment.
additionally noticed one thing, jfyi
we have to be care with Intergalactic.Compoent type as prop. That is why React.ComponentType<NoticeSmartProps> is used in that context. The downside of that approach is that it could miss internal props like tag render, etc...
Example:
type Props = {
Test: Intergalactic.Component<'div', NoticeSmartProps>;
}I expect that only components that implement NoticeSmartProps will be a valid input (yeah, I know that NoticeSmartProps is extended by BoxProps and vast majority of components are extended too..., but...)
POV:
<Component Test={Box} />and TS won't tell anything :)
| <Notice.Close onClick={onNotificationClose} /> | ||
| </Notice> | ||
|
|
||
| <Notice.Text tag={Flex} gap={3} alignItems={notificationTitle ? 'flex-start' : 'center'}> |
There was a problem hiding this comment.
I think - this is incorrect to use Notice.Text here - it should be NoticeComponent.Text.
I see the problem, maybe we need to transform a little Notice in FeatureHighlight - add some advanced mode or something like this to use advanced Notice.
There was a problem hiding this comment.
updated
| } | ||
|
|
||
| SFeedbackRatingNoticeComponent > *[data-ui-name="Notice.Label"] { | ||
| margin-right: var(--intergalactic-spacing-3x, 12px); |
There was a problem hiding this comment.
And after replacing the Notice, you don't need to use this style
There was a problem hiding this comment.
deleted
… advanced mode/update FeedbackRating component to accept Notice & Illustration components/updated snapshots
| const { styles, Children } = this.asProps; | ||
|
|
||
| const advancedMode = isAdvanceMode(Children, [ | ||
| NoticeFH.Label.displayName, |
There was a problem hiding this comment.
@ilyabrower idk, shall we check all inner component to enable advanced mode?
| Title: typeof Text; | ||
| Text: typeof Text; | ||
| Close: typeof Button; | ||
| /** |
There was a problem hiding this comment.
should be added as well just to preserve the same type as Notice has
Changelog
@semcore/feedback-form
Added
illustrationElement,NoticeComponentforFeedbackRatingcomponent.@semcore/feature-highlight
Added
Notice.Motivation and Context
How has this been tested?
Screenshots (if appropriate):
Types of changes
Checklist: