-
Notifications
You must be signed in to change notification settings - Fork 55
fix(factories): add types to createShorthandFactory() methods #1875
Conversation
packages/react/src/lib/factories.ts
Outdated
@@ -91,6 +92,10 @@ type CreateShorthandFactoryConfigInner<TPropName = string> = { | |||
mappedArrayProp?: TPropName | |||
} | |||
export type CreateShorthandFactoryConfig = CreateShorthandFactoryConfigInner | |||
export type CreateShorthandFactoryResult = ( | |||
a: ShorthandValue<UIComponentProps>, |
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.
a: ShorthandValue<UIComponentProps>, | |
value: ShorthandValue<UIComponentProps>, |
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.
a: ShorthandValue<UIComponentProps>, | |
a: ShorthandValue<P>, |
Actually, let's make it generic and accept component props there. Hope that it will work
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.
Yes it works
Codecov Report
@@ Coverage Diff @@
## master #1875 +/- ##
======================================
Coverage 69.7% 69.7%
======================================
Files 886 886
Lines 7797 7797
Branches 2258 2258
======================================
Hits 5435 5435
Misses 2352 2352
Partials 10 10
Continue to review full report at Codecov.
|
…o "value, options", avoid cirtular reference in factories.ts
@@ -214,7 +215,8 @@ class MenuItem extends AutoControlledComponent<WithAsProp<MenuItemProps>, MenuIt | |||
} = this.props | |||
const { menuOpen } = this.state | |||
|
|||
const indicatorWithDefaults = indicator === undefined ? {} : indicator | |||
const defaultIndicator = { name: vertical ? 'stardust-arrow-end' : 'stardust-arrow-down' } | |||
const indicatorWithDefaults = indicator === undefined ? defaultIndicator : indicator |
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.
Are these changes required? Can we revert them?
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.
It is required, because Type {}
is not assignable to type Props<IconProps>
.
@@ -106,7 +107,8 @@ class AccordionTitle extends UIComponent<WithAsProp<AccordionTitleProps>, any> { | |||
|
|||
renderComponent({ ElementType, classes, unhandledProps, styles, accessibility }) { | |||
const { contentRef, children, content, indicator, active } = this.props | |||
const indicatorWithDefaults = indicator === undefined ? {} : indicator | |||
const defaultIndicator = { name: active ? 'stardust-arrow-down' : 'stardust-arrow-end' } | |||
const indicatorWithDefaults = indicator === undefined ? defaultIndicator : indicator |
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.
Another PR: |
Fixes #1865
The new type is
Won't compile
Will compile
And this will unfortunately also compile