jeremyckahn / farmhand

A resource management game that puts a farm in your hand
https://www.farmhand.life
GNU General Public License v2.0
101 stars 28 forks source link

feat: add total price to item #480

Closed tim-phillips closed 10 months ago

tim-phillips commented 10 months ago

What this PR does

This PR adds total price next to item price in the Item card when quantity is greater than one.

How this change can be validated

Go to the store and view the Seeds tab, click the up arrow on a seed card.

In the inventory on the left sidebar, click the up arrow on a card that has the option of selling more than one.

Questions or concerns about this change

For sell price, I'm a little concerned about the text under the arrow icons. And I'm not sure what happens if the text content runs into the edge of the card.

Additional information

Screenshot 2024-01-29 at 4 54 19 PM Screenshot 2024-01-29 at 4 54 27 PM

Resolves: #479

vercel[bot] commented 10 months ago

Someone is attempting to deploy a commit to a Personal Account owned by @jeremyckahn on Vercel.

@jeremyckahn first needs to authorize it.

vercel[bot] commented 10 months ago

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

Name Status Preview Comments Updated (UTC)
farmhand ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2024 2:13am
tim-phillips commented 10 months ago

Glad to make the change! I agree that the button overlap issue needs addressing. I futzed around with the tests but couldn't achieve anything useful with enzyme. I'm glad to hear that other tests are using RTL! I'll take that approach and add some here.

Since quantity can be undefined when the user first clicks on the input, the calculation will result in NaN, which is why I opted to only show the total when quantity is greater than 1. With this change, in order to prevent line jumps and card resizing I can show "Total" all of the time. What do you think?

Also, would "Total sell price" be better copy?

Screenshot 2024-01-30 at 10 30 33 AM Screenshot 2024-01-30 at 10 30 44 AM Screenshot 2024-01-30 at 10 30 51 AM
jeremyckahn commented 10 months ago

With this change, in order to prevent line jumps and card resizing I can show "Total" all of the time. What do you think?

For this first iteration, let's go with just showing "Total" at all times to avoid the line jumping. We can improve that later if it proves to be annoying!

tim-phillips commented 10 months ago

I added some tests and changed Total to live on its own line for both buy and sell. I think it's a great improvement. Thanks for talking all that through!

jeremyckahn commented 10 months ago

Thanks for the updates @tim-phillips! I don’t have much free time in the next few days, but I will prioritize reviewing this when I can.