Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(application-components): to convert to TS #921

Merged
merged 3 commits into from
Aug 5, 2019

Conversation

emmenko
Copy link
Member

@emmenko emmenko commented Aug 2, 2019

Now that we have parts of uikit typed (#919) we can put them into use for the application components. In fact, I already found some bugs! 😮

<ClassNames>
{({ css: makeClassName }) => (
<Modal
isOpen={forceClose === true ? false : props.isOpen}
Copy link
Member Author

@emmenko emmenko Aug 2, 2019

Choose a reason for hiding this comment

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

Now that we useState, it was a bit weird before to use null and false as the state values. Now it's either true or false, the outcome should be the same though.

@emmenko emmenko force-pushed the nm-ts-application-components branch from cf5c75d to 6f248e2 Compare August 2, 2019 20:57
@emmenko emmenko force-pushed the nm-ts-application-components branch from 59273a7 to f8b1f4b Compare August 2, 2019 21:37
@emmenko emmenko marked this pull request as ready for review August 2, 2019 21:52
@emmenko emmenko requested review from montezume and jonnybel August 2, 2019 22:04
@emmenko
Copy link
Member Author

emmenko commented Aug 5, 2019

@montezume @jonnybel it would be great if you can have a look a this today, thanks

@montezume
Copy link
Contributor

Will do 👍

Copy link
Contributor

@jonnybel jonnybel left a comment

Choose a reason for hiding this comment

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

Ok, I went through the whole PR, which for me it was mostly a TS learning by example experience, and I didn't find anything wrong.

type DialogControllerProps = {
children: (renderProps: {
isOpen: boolean;
setIsOpen: React.Dispatch<boolean>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find anything in google (not redux related) about React.Dispatch. can you enlighten?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay. weird

would it be more readable to just put (value: A) => void ?

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 is just for a test btw, I just went with the same API used by useState.

Also, if you have TS editor installed, you can see the type signatures:
image

Copy link
Contributor

@montezume montezume left a comment

Choose a reason for hiding this comment

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

Everything looks good to me too

@emmenko emmenko merged commit 9cd24d3 into master Aug 5, 2019
@emmenko emmenko deleted the nm-ts-application-components branch August 5, 2019 13:25
@emmenko emmenko mentioned this pull request Aug 6, 2019
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants