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: add types util #409

Merged
merged 2 commits into from
May 20, 2024
Merged

refactor: add types util #409

merged 2 commits into from
May 20, 2024

Conversation

lissavxo
Copy link
Collaborator

Related to #405

Description

This adds a file with the types being used in the codebase

Test plan

yarn build && yarn dev

@lissavxo lissavxo requested review from Klakurka and chedieck May 17, 2024 07:30
Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

Left a comment, also:

In order to avoid having to deal with conflicts later on, I'd suggest that you e.g merge #407 into this branch and put in the description that it depends on #407 being merged so it can be reviewed.

If you don't want to do that it's fine of course, but after merging #407 and merging master into here, there are gonna be conflicts that will need to be solved, so in a way this depends in #407 being solved anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we put everything here UpperCamelCase? I think it's an easy change, besides currencyObject which will be tricky because, across the codebase, sometimes is a variable and sometime is a type, but that is why we shouldn't even have it camelCased in the first place.

@lissavxo lissavxo force-pushed the feat/add-types-util branch from b4c6f29 to 143a79f Compare May 18, 2024 20:25
@lissavxo lissavxo requested a review from chedieck May 18, 2024 20:26
@Klakurka Klakurka merged commit b8666b0 into master May 20, 2024
1 check passed
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.

3 participants