Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(mixed): correct types to match propTypes #550

Merged
merged 8 commits into from
Dec 20, 2018
Merged

fix(mixed): correct types to match propTypes #550

merged 8 commits into from
Dec 20, 2018

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Dec 3, 2018 •
edited
Loading

Fixes #548.

TS sandbox with types, don't forget to enable strictNullChecks in options 👍

@layershifter layershifter changed the title Fix/update typings fix(mixed): corrent types to match propTypes Dec 3, 2018
kuzhelov
kuzhelov reviewed Dec 3, 2018
View reviewed changes
src/components/Attachment/Attachment.tsx Outdated
import Icon from '../Icon/Icon'
import Button from '../Button/Button'
import Text from '../Text/Text'
import Slot from '../Slot/Slot'

export interface AttachmentProps extends UIComponentProps, ChildrenComponentProps {
/** Button shorthand for the action slot. */
action?: ShorthandValue
action?: Nullable<ShorthandValue>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it would be easier to use generic type that will be able to transform entire props type, like that:

export interface AttachmentProps {
  action?: ShorthandValue,
  ...
}

class Attachment extends UIComponent<Nullable<AttachmentProps>, ...>

@layershifter layershifter added 🚧 WIP 🧰 fix Introduces fix for broken behavior. labels Dec 4, 2018
@layershifter layershifter force-pushed the fix/update-typings branch 2 times, most recently from 6bb0eba to a95e382 Compare December 4, 2018 16:30
@layershifter
Copy link
Member Author

I don't understand this:

image

image

🦀

@layershifter layershifter changed the title fix(mixed): corrent types to match propTypes fix(mixed): correct types to match propTypes Dec 5, 2018
layershifter
layershifter commented Dec 5, 2018
View reviewed changes
types/utils.d.ts
@@ -24,6 +27,10 @@ export type ObjectOrFunc<TResult, TArg = {}> = ((arg: TArg) => TResult) | TResul

export type Props = ObjectOf<any>
export type ReactChildren = React.ReactNodeArray | React.ReactNode

export type ReactPropsStrict<T> = { [K in keyof T]: NullableIfUndefined<T[K]> }
export type ReactProps<T> = Extendable<ReactPropsStrict<T>>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a small difference, ReactPropsStrict cannot be extended it's very useful for Portal, Ref and other component that don't allow to spread props.

@layershifter layershifter mentioned this pull request Dec 10, 2018
Merged
@layershifter layershifter force-pushed the fix/update-typings branch 2 times, most recently from 283ee27 to 8ca9f2a Compare December 13, 2018 15:53
@layershifter
Copy link
Member Author

#608 showed that a build can be passed, I will try to fix all remaining errors to be compatible with the latest TS.

layershifter
layershifter commented Dec 14, 2018
View reviewed changes
docs/src/examples/components/Ref/Types/RefForwardingExample.tsx Outdated
@@ -1,7 +1,7 @@
import React from 'react'
import { Grid, Ref, Segment } from '@stardust-ui/react'

const ExampleButton = React.forwardRef<HTMLButtonElement>((props, ref) => (
const ExampleButton = React.forwardRef<HTMLButtonElement, { children: string }>((props, ref) => (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It becomes more problematic, we should force #495.

layershifter
layershifter commented Dec 14, 2018
View reviewed changes
docs/src/prototypes/popups/GridImagePicker/GridImagePickerItem.tsx

return (
<ElementType>
<li>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't make any reasonable sense, but caused my favourite error, microsoft/TypeScript#28768

layershifter
layershifter commented Dec 14, 2018
View reviewed changes
src/lib/accessibility/FocusZone/FocusTrapZone.types.tsx
@@ -1,3 +1,5 @@
import * as React from 'react'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import was absent before

miroslavstastny
miroslavstastny approved these changes Dec 19, 2018
View reviewed changes
types/utils.d.ts
@@ -24,6 +27,10 @@ export type ObjectOrFunc<TResult, TArg = {}> = ((arg: TArg) => TResult) | TResul

export type Props = ObjectOf<any>
export type ReactChildren = React.ReactNodeArray | React.ReactNode

export type ReactPropsStrict<T> = { [K in keyof T]: NullableIfUndefined<T[K]> }
export type ReactProps<T> = Extendable<ReactPropsStrict<T>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when I should use Extendable vs ReactProps? My understanding is I should always use ReactProps (or ReactPropsStrict in special cases).
But for example Dropdown is still using Extendable.
Shouldn't we remove Extendable completely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropdown should use ReactProps, too. I missed it, will check all Dropdown's components.

Shouldn't we remove Extendable completely?

Note sure, it's a quite generic type, we're using it in theme/types.ts for colors and props.

@layershifter
Merge branches 'fix/update-typings' and 'master' of https://github.co…
696782b 
…m/stardust-ui/react into fix/update-typings

# Conflicts:
#	docs/src/examples/components/Ref/Types/RefForwardingExample.tsx
#	src/components/List/List.tsx
#	src/components/List/ListItem.tsx
#	src/components/Menu/Menu.tsx
#	src/components/Menu/MenuItem.tsx
@layershifter
use ReactProps in Dropdown
af30cbd
@layershifter
fix proptypes imports
5a93673
@layershifter
Merge branches 'fix/update-typings' and 'master' of https://github.co…
f17c28b 
…m/stardust-ui/react into fix/update-typings
@layershifter layershifter merged commit 7893354 into master Dec 20, 2018
@layershifter layershifter deleted the fix/update-typings branch December 20, 2018 11:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kuzhelov kuzhelov kuzhelov left review comments

@miroslavstastny miroslavstastny miroslavstastny approved these changes

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@levithomason levithomason Awaiting requested review from levithomason levithomason is a code owner

@mnajdova mnajdova Awaiting requested review from mnajdova

Assignees
No one assigned
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@layershifter @kuzhelov @miroslavstastny