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: Improve typing so it shows the props #1591

Merged
merged 7 commits into from
Dec 8, 2021

Conversation

massao
Copy link
Contributor

@massao massao commented Dec 6, 2021

Before
With these changes instead of the IDE showing:
image

After
It will expand the props and show like this:
image

Currently I'm checking if it's possible to get rid of the n more and show all of them, but seems like a configuration on the editor side
It's actually on the tsserver file used on the VSCode, it isn't even a configuration that can be changed by the user without changing the file itself, for more context check here

PS: Only did the changes on the Button package for now, while I'm checking how to get that improved.
PS2: If you try this branch locally you may need to rebuild the packages and maybe restart the TS server.

@netlify
Copy link

netlify bot commented Dec 6, 2021

✔️ Deploy Preview for forma-36 ready!

🔨 Explore the source changes: 751e39d

🔍 Inspect the deploy log: https://app.netlify.com/sites/forma-36/deploys/61b0ea9a02e4c30007accdee

😎 Browse the preview: https://deploy-preview-1591--forma-36.netlify.app

@@ -1,5 +1,11 @@
import React from 'react';

export type ExpandProps<T> = T extends object
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, this is quite cool. Now I need to understand how it works :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What this does is get the type T and if it's an object, it creates a copy and iterate over and returns a object typing {}, so it shows up as that on the definition not as the name.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the explanation

Copy link
Contributor

@bgutsol bgutsol Dec 7, 2021

Choose a reason for hiding this comment

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

Could you add some explanation around this type in the code comment, like the purpose and how it works (pretty much your answer for Alex) for the future us.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 ☝️

import getStyles from './ButtonGroup.styles';
import type { ButtonGroupProps } from './types';

function _ButtonGroup(props: ButtonGroupProps, ref: React.Ref<HTMLDivElement>) {
function _ButtonGroup(
props: ExpandProps<ButtonGroupProps>,
Copy link
Contributor

Choose a reason for hiding this comment

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

can ExpandProps be a part of ButtonGroupProps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly no, because it would expand the Type inside the ButtonGroupProps, but here it would show as ButtonGroupProps only, and not the values inside it

@domarku
Copy link
Contributor

domarku commented Dec 7, 2021

This is so much better 😍

Copy link
Contributor

@suevalov suevalov left a comment

Choose a reason for hiding this comment

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

I think it's an obvious improvement compared to what we currently have, so let's go for it.

Copy link
Contributor

@bgutsol bgutsol left a comment

Choose a reason for hiding this comment

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

Much better for user experience 👍
And codewise, I think, it's alright. Our polymorphic types are already a pretty magical thing, it won't make it worse 😄

Copy link
Contributor

@thebesson thebesson left a comment

Choose a reason for hiding this comment

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

100% an improvement!

Copy link
Contributor

@gui-santos gui-santos left a comment

Choose a reason for hiding this comment

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

🧙

@@ -1,5 +1,11 @@
import React from 'react';

export type ExpandProps<T> = T extends object
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 ☝️

Copy link
Contributor

@burakukula burakukula left a comment

Choose a reason for hiding this comment

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

💪🧙‍♂️ Great improvement! ❤️ it!

@massao massao marked this pull request as ready for review December 8, 2021 14:55
@massao massao requested review from suevalov and domarku December 8, 2021 14:55
@massao massao enabled auto-merge (squash) December 8, 2021 16:03
@massao massao merged commit bb11d4a into forma-v4 Dec 8, 2021
@massao massao deleted the refactor/improve-typing branch December 8, 2021 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants