Skip to content
New issue

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

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

Already on GitHub? # to your account

feat: add expense tracker to kitchen skin #655

Merged
merged 3 commits into from
Oct 6, 2023
Merged

feat: add expense tracker to kitchen skin #655

merged 3 commits into from
Oct 6, 2023

Conversation

vimode
Copy link
Contributor

@vimode vimode commented Oct 4, 2023

Please describe the changes this PR makes and why it should be merged:

This PR adds Expense tracker example to the list of Kitchen Sink examples as per #606

I ran the pnpm cleanup command and it modified a lot of files, but I only pushed the changes for this project file and nothing more, hope thats okay.

The pnpm lint and pnpm test command and all seems OK.

My knowledge of TS and million is very basic so please recommend fixes as needed.

If there is any missing feature in the current implementation of the project or some logic that seems odd or need fixing, I am ready to work on it as needed.

Thank you.

Status

  • Code changes have been tested against prettier, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR only includes non-code changes, like changes to documentation, README, etc.

@vercel
Copy link

vercel bot commented Oct 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
million ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 5, 2023 6:59am
million-kitchen-sink ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 5, 2023 6:59am
sink ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 5, 2023 6:59am

@CLAassistant
Copy link

CLAassistant commented Oct 4, 2023

CLA assistant check
All committers have signed the CLA.

@tobySolutions
Copy link
Contributor

Please describe the changes this PR makes and why it should be merged:

This PR adds Expense tracker example to the list of Kitchen Sink examples as per #606

I ran the pnpm cleanup command and it modified a lot of files, but I only pushed the changes for this project file and nothing more, hope thats okay.

The pnpm lint and pnpm test command and all seems OK.

My knowledge of TS and million is very basic so please recommend fixes as needed.

If there is any missing feature in the current implementation of the project or some logic that seems odd or need fixing, I am ready to work on it as needed.

Thank you.

Status

  • Code changes have been tested against prettier, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR only includes non-code changes, like changes to documentation, README, etc.

Sure thing! @vimode! Reviewing now

Copy link
Contributor

@tobySolutions tobySolutions left a comment

Choose a reason for hiding this comment

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

LGTM! Interesting usage of the block function though. Is there a reason as to your usage in just the ExpenseItem

@vimode
Copy link
Contributor Author

vimode commented Oct 5, 2023

Hey @tobySolutions

Thank you for the review.

If I use the block function on the whole app. the way I have the form inputs be handled by the parent component causes the whole app to re-render(?) so when one starts typing, it loses focus on the input on every key stroke.

But I think moving the state locally to the component should fix that issue. Let me try and fix this accordingly.

@vimode
Copy link
Contributor Author

vimode commented Oct 5, 2023

Hey @tobySolutions I millionified it a bit more! 🦁

@tobySolutions
Copy link
Contributor

Good work @vimode!

Copy link
Contributor

@tobySolutions tobySolutions left a comment

Choose a reason for hiding this comment

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

LGTM!!

@tobySolutions tobySolutions merged commit 1a9f9e9 into aidenybai:main Oct 6, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants