zengm icon indicating copy to clipboard operation
zengm copied to clipboard

Add total salary of selected players for 'Trading Block Page'

Open fearandesire opened this issue 1 year ago • 1 comments

Hi,

This PR is concerning this TODO

I've added this simple text to show the contract sum of the current players selected. You can preview what it looks like in this image The code and display for it isn't perfect, and I'm open to making adjustments as @dumbmatter. I also understand if this feature isn't necessarily what should be added at this time, and OK with this being scrapped.

fearandesire avatar Jun 07 '23 23:06 fearandesire

Thanks for doing this!

A couple changes would be nice before merging:

  1. Rather than adding two new state variables and a new callback function to keep it in sync, just add up the contract sum in the body of the TradingBlock component. This will be shorter code, will be plenty fast, and is also generally what people recommend for React these days, for example https://kentcdodds.com/blog/dont-sync-state-derive-it

  2. Maybe move the sum somewhere else. Maybe right below/above the submit button? I'm not sure, it just looks a little out of place at the top.

dumbmatter avatar Jun 12 '23 21:06 dumbmatter