Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update unit scroller behavior when no moves left & fix alert button getting focus #6153

Merged
merged 2 commits into from
Apr 7, 2020
Merged

Update unit scroller behavior when no moves left & fix alert button getting focus #6153

merged 2 commits into from
Apr 7, 2020

Conversation

DanVanAtta
Copy link
Member

commit 95649dc

Update unit scroller behavior when no moves left

1) When no moves left:
- do not render the territory label
- if skipping the last unit, getting to zero moves
  left, then remove the unit avatar rendering image

2) Also, (cleanup) remove an unnecessary update to
   'movesLeft' in territory listener. Changing territory
   hover should not change the moves left and is not a
   necessary invocation.

commit 7910ed8

Remove focusable from 'alert' button

Clicking the alert button repeatedly has no real effect, a user
typically does not want to do this. After a first click, the button
gains focus. *Then* pressing space will activate the button. This
is confusing behavior as the next available unit is selected
and it seems like we are 'skipping' units with the space button
but we are really pressing 'wake-all' repeatedly.

To avoid this confusing scenario, remove focusable from 'alert'
button. This way if it is clicked, and then a user presses space,
that will activate the 'skip' button.

Functional Changes

[] New map or map update
[] New Feature
[x] Feature update or enhancement
[] Feature Removal
[] Code Cleanup or refactor
[] Configuration Change
[] Problem fix:
[] Other:

Testing

  • some play testing

1) When no moves left:
- do not render the territory label
- if skipping the last unit, getting to zero moves
  left, then remove the unit avatar rendering image

2) Also, (cleanup) remove an unnecessary update to
   'movesLeft' in territory listener. Changing territory
   hover should not change the moves left and is not a
   necessary invocation.
Clicking the alert button repeatedly has no real effect, a user
typically does not want to do this. After a first click, the button
gains focus. *Then* pressing space will activate the button. This
is confusing behavior as the next available unit is selected
and it seems like we are 'skipping' units with the space button
but we are really pressing 'wake-all' repeatedly.

To avoid this confusing scenario, remove focusable from 'alert'
button. This way if it is clicked, and then a user presses space,
that will activate the 'skip' button.
@codeclimate
Copy link

codeclimate bot commented Apr 7, 2020

Code Climate has analyzed commit 7910ed8 and detected 0 issues on this pull request.

View more on Code Climate.

@codecov
Copy link

codecov bot commented Apr 7, 2020

Codecov Report

Merging #6153 into master will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6153      +/-   ##
============================================
- Coverage     22.95%   22.94%   -0.02%     
- Complexity     6598     6599       +1     
============================================
  Files          1104     1104              
  Lines         78344    78375      +31     
  Branches      11412    11415       +3     
============================================
- Hits          17983    17980       -3     
- Misses        58166    58197      +31     
- Partials       2195     2198       +3     
Impacted Files Coverage Δ Complexity Δ
...trategy/triplea/ui/unit/scroller/UnitScroller.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ine/message/unifiedmessenger/UnifiedMessenger.java 67.68% <0.00%> (-7.49%) 27.00% <0.00%> (+2.00%) ⬇️
...ames/strategy/engine/message/RemoteMethodCall.java 85.07% <0.00%> (-5.41%) 32.00% <0.00%> (+1.00%) ⬇️
.../triplea/delegate/battle/UnitBattleComparator.java 31.09% <0.00%> (-1.69%) 14.00% <0.00%> (-2.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96000cf...7910ed8. Read the comment docs.

Copy link
Member

@RoiEXLab RoiEXLab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was so confused. I was asking myself, what are movesLeft supposed to be and why aren't there any movesRight?
We'll after realizing my mistake I suggest renaming the fields and methods to movesRemaining or something 😅

@DanVanAtta
Copy link
Member Author

😄 Nothing really coming to mind for a better name without getting super wordy. movesRemaining invites a problem where it seems like a list of moves rather than a count. So we then get to movesRemainingCount, which then leads to how do we rename the "updateMovesLeft()" method as well.

@DanVanAtta DanVanAtta merged commit 837c68c into triplea-game:master Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants