Updates to UI to show how many units can be purchased. #11273
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #11273 +/- ##
============================================
- Coverage 27.21% 27.20% -0.01%
+ Complexity 8251 8249 -2
============================================
Files 1217 1217
Lines 78080 78088 +8
Branches 10615 10616 +1
============================================
- Hits 21250 21246 -4
- Misses 54862 54875 +13
+ Partials 1968 1967 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@jtkenny nice 👍 It's been a few days so lets go ahead and ping @DanVanAtta . He hasn't been active lately but I know he's interested in your work. Will ping @RoiEXLab too :) |
|
@jtkenny From a coding perspective the changes seem fine. Please fix the formatting violations by running @beelee1 @jtkenny Can you give me some background on this change? Screenshots of the "before" and "after" state would be super useful, because I'm not familiar with those specific parts of the code |
|
@RoiEXLab |
|
@TheDog-GH Thanks for clarifying. In this case I'm just asking for the screenshots (as mentioned earlier) for future reference and to fix the formatting. 👍🏼 |
|
Before: After: |
| final Action doneAction = SwingAction.of("Done", () -> dialog.setVisible(false)); | ||
|
|
||
| ProductionPanel(final UiContext uiContext) { | ||
| this.uiContext = uiContext; | ||
| lowestCost = 100000; |
There was a problem hiding this comment.
One thing I forgot to ask: Where does this value of 100k come from? Is it just an arbitrarily high number or is there a deeper meaning?
There was a problem hiding this comment.
Arbitrary high number. If it was zero, then if the associated if statement was never triggered it would cause divide by zero.
There was a problem hiding this comment.
Ah okay, in this case is something speaking against using Integer.MAX_VALUE? Also if you want to can assign the value in the same line where you define the variable. No need to put it into the constructor
|
Thanks for taking a whack at this update and the work you have put in. Few questions: (1) Have you considered any other UI treatments? I'm not sure we want to go with the UI treatment suggested. What bugs me is how it groups visually in my brain, it looks like "1 | 2" and "1 < 10" to me. If you are short on ideas for alternatives, there are a few UI designer folks who hang out on the forums who likely would provide more than feedback and options than you could shake a stick at. (2) Are there any functionality changes to limit the max from being entered? For example, does the "up" arrow toggle become disabled when at the max? If a user enters in a value over the max, is it overwritten with the max? (3) How extensively have edge cases been considered? Notably I'm thinking of maps where there is placement that is outside of factories (one example is placing allied units on china I think in BWv3 maps). Have you checked additional maps like civil war and others that have some extra variation for unit placement? My availability has been pretty poor lately. Apologies in advance for any delayed responses. Otherwise, generally I think we should focus on:
|
|
Ah, just noticed an interesting thing here is the difference between max able to purchase (due to cost) and the max that could be purchased due to placement. I think the latter is the really interesting number. |
|
Please re-open @jtkenny when you're ready & consider marking this PR as a draft as well. Given it's a been a month without any update, I'm marking this as closed to remove it from the active queue. |
|
@asvitkine |
Change Summary & Additional Notes
Release Note
This is the pull request for the changes done by the ISU students. This contains the UI updates requested on the forum for displaying the total number of units that can be bought. The related issue is here: #11258