feat: add total price to item #480
Conversation
|
Someone is attempting to deploy a commit to a Personal Account owned by @jeremyckahn on Vercel. @jeremyckahn first needs to authorize it. |
tim-phillips
changed the title
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Thanks for making this addition, @tim-phillips! The code here looks good, but I'd like to get the button overlap issue resolved before merging this. For the Sell view, I think wrapping the parenthesized section to the next line should be sufficient.
Also, this seem like something worth adding test coverage for. We have src/components/Item/Item.test.js in place, but that file is using Enzyme (it's old...). These days, Farmhand's new tests use React Testing Library. We can either add RTL tests alongside the Enzyme tests in the same file, or we can spin up a new file for RTL tests at src/components/Item/Item.rtl.test.js (something we've done for other components that have a mix of Enzyme and RTL tests).
Let me know if you'd like some support with making these changes!
|
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 Also, would "Total sell price" be better copy? 
|
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! |
|
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! |
|
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. |
There was a problem hiding this comment.
LGTM! Thanks for this excellent addition to Farmhand, @tim-phillips.
I just realized that I missed this question:
Also, would "Total sell price" be better copy?
In the interest of minimalism, I think that "Total" as you have it now should be fine. We can revisit this in later iterations if we end up not liking it.
I think this is ready to merge, so I will do so now and release it as version 1.18.9.
Congrats on your first contribution to Farmhand! 🎉
| import CardHeader from '@mui/material/CardHeader' | ||
| import { shallow } from 'enzyme' |
There was a problem hiding this comment.
Amazing! Thank you for getting Enzyme out of this file. 🙏
| description: '', | ||
| doesPriceFluctuate: false, | ||
| isReplantable: false, |
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
Resolves: #479