the-collab-lab / tcl-28-smart-shopping-list

1 stars 3 forks source link

Some small changes based on comments in sd-ia-estimate-next-purchase PR 👍 #33

Closed isaabutaa closed 3 years ago

isaabutaa commented 3 years ago

Description

Made a couple small changes based on @ksiman14's comments on sd-ia-estimate-next-purchase PR.

NOTE: I did notice that since we are using the calculateEstimate function inside a handleChange, the estimated next purchase is calculate both when the box is checked and unchecked. So we may need to do something small to refactor.

If things changes are okay and we all agree, maybe we can merge this with the sd-ia-estimate-next-estimate branch before merging that branch to main.

github-actions[bot] commented 3 years ago

Visit the preview URL for this PR (updated for commit a9430c4):

https://tcl-28-shopping-list--pr33-experimenting-with-e-80rnrgs9.web.app

(expires Thu, 19 Aug 2021 12:50:13 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

isaabutaa commented 3 years ago

Yes @sandaiiyahh I think that's a great idea! 💯 I'll see if I can implement that condition

isaabutaa commented 3 years ago

@sandaiiyahh I put a ternary statement in the .update() for the daysUntilPurchase value checking to see if the checkbox is checked like you suggested. 👇

daysUntilPurchase: e.target.checked ? nextPurchaseDate : daysUntilPurchase

My code editor did an auto format, so the ternary is on multiple lines. But it works! daysUntilPurchase gets updated now only if the box is being checked, not unchecked.

sandaiiyahh commented 3 years ago

@sandaiiyahh I put a ternary statement in the .update() for the daysUntilPurchase value checking to see if the checkbox is checked like you suggested. 👇

daysUntilPurchase: e.target.checked ? nextPurchaseDate : daysUntilPurchase

My code editor did an auto format, so the ternary is on multiple lines. But it works! daysUntilPurchase gets updated now only if the box is being checked, not unchecked.

Hmm, I think it will still calculate the estimate under the hood whenever it's checked or unchecked. But yes, that works too!

isaabutaa commented 3 years ago

Hmm, I think it will still calculate the estimate under the hood whenever it's checked or unchecked. But yes, that works too!

That's a really good point! I hadn't considered that.

isaabutaa commented 3 years ago

Hmm, I think it will still calculate the estimate under the hood whenever it's checked or unchecked. But yes, that works too!

@sandaiiyahh I updated the code so that calculateEstimate is inside a condition statement. Should be good now. 😎

let nextPurchaseDate; if (e.target.checked) { nextPurchaseDate = calculateEstimate( daysUntilPurchase, latestInterval, numberOfPurchases + 1, ); }