-
Notifications
You must be signed in to change notification settings - Fork 101
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
Move types to dedicated file #168
Conversation
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.
LGTM 🚀
Just be sure to rebase/merge back your up to date main into this branch and resolve the conflicts 👍
…into store-types-seperately
…into store-types-seperately
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.
@h1divp I'm not the most familiar with the props/types, does this look good to you?
Architecture-wise I think this is a good change. Moving types into one file seems to be what some companies do, and I think the real benefit comes from easily seeing when we can consolidate multiple types into one, which we might want to make an issue for after this is merged (ex. all of the button props types can just be one type instead of 3 of them). I am going to update this branch, resolve conflicts, and fix any broken functionality (should there be any) at CC tomorrow. |
I do also think this needs to be part of our style guide, or referenced in the docs though. So we can prevent people from adding interfaces within random files instead of Props.ts and importing them. |
The code is a bit outdated. Please rebase or merge with your forks main! |
I'll work on this today |
aaaaaaaaaaaaaaaaaaaaa |
Nice organization, functionality works as expected! |
Resolves #122
The props for most of the components in the Common folder have been moved into
utils/types.ts
and exported from there. This follows a pattern similar to what we have done for our server-side scripts.