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

fix: separated vestingSchedule #458

Merged
merged 11 commits into from
Jul 25, 2024

Conversation

Alfredoeb9
Copy link
Contributor

Issue

Issue: #454

Description

separated vestingSchedule into cliffYears and vestingYears

updated prisma tables for Share and Options to separate vestingSchedule into cliffYears and vestingYears with respectable trpc types and updated Shares and Options modal to separate vestingSchedule into two columns cliffYears and vestingYears

separated vestingSchedule into cliffYears and vestingYears

updated prisma tables for Share and Options to separate vestingSchedule into cliffYears and vestingYears with respectable trpc types and updated Shares and Options modal to separate vestingSchedule into two columns cliffYears and vestingYears
Copy link

github-actions bot commented Jul 21, 2024

Thank you for following the naming conventions for pull request titles! 🙏

@Alfredoeb9 Alfredoeb9 changed the title [vesting schedule] separated vestingSchedule fix: separated vestingSchedule Jul 21, 2024
Copy link
Contributor

@dahal dahal left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR @Alfredoeb9.

There are some build errors

CleanShot 2024-07-21 at 04 39 42

You can always make sure the build works by running pnpm build

fixed build errors, had to remove vestingSchedule from GET request on getSharesProcedure and getOptionsProcedure and replace with cliffYears and vestingYears
removed vestingSchedule at server level and replaced with cliffYears and vestingYears as vestingSchedule was separted into two fields
@Alfredoeb9 Alfredoeb9 requested a review from dahal July 21, 2024 16:14
updated default value from empty string to 0 as default types for fields are numbers
Copy link
Contributor

@dahal dahal left a comment

Choose a reason for hiding this comment

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

@Alfredoeb9
Copy link
Contributor Author

Fixed build errors mentioned above and tested build and passed with npm build command and with docker compose up --build

@Alfredoeb9 Alfredoeb9 requested a review from dahal July 22, 2024 22:44
@Alfredoeb9
Copy link
Contributor Author

Alfredoeb9 commented Jul 22, 2024

Getting inconsistent webpack build errors when running docker compose up nuking branch and setting up again

image

I nuked my local project and cloned again ran docker compose up and still received same webpack error as above.

I nuked the local project again and cloned, I then reverted back one commit when I did not see webpack errors from screenshot, ran docker compose up and received same webpack error.

I nuked the local project again and cloned, I then reverted back two commits when I did not see the webpack errors from screenshot, ran docker compose up and received same webpack error.

With these findings i'm not sure what could have caused the error on screenshot.

@Alfredoeb9
Copy link
Contributor Author

After updating docker application, I am no longer seeing random webpack errors and application is running as expected locally.

Successful build on pnpm build and docker compose up --build

Copy link
Contributor Author

@Alfredoeb9 Alfredoeb9 left a comment

Choose a reason for hiding this comment

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

Review Completed

Copy link
Contributor

@dahal dahal left a comment

Choose a reason for hiding this comment

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

🚀

@dahal dahal merged commit e769873 into captableinc:main Jul 25, 2024
2 checks passed
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.

2 participants