Skip to content

Improve usability of create game form #11568

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

garrettngarcia
Copy link

Fixes #11071

Before clicking "Create"

  • Branch is derived from the latest master
  • Add the pins or archive_alpha_games label if this change will break existing games
  • Code passes linter with docker compose exec rack rubocop -a
  • Tests pass cleanly with docker compose exec rack rake

Implementation Notes

Explanation of Change

Now selects number input field value on focus. Also added min and max player info to the form labels.

Tested on Safari and Chrome on iOS and Mac.

Screenshots

Any Assumptions / Hacks

Fixes tobymao#11071. Now selects number input field value
on focus.  Also added min and max player info to
the labels.
@garrettngarcia garrettngarcia marked this pull request as ready for review February 20, 2025 06:49
@ollybh ollybh added the front page Issues relating front page, login, game creation etc label Feb 20, 2025
Copy link
Collaborator

@ollybh ollybh left a comment

Choose a reason for hiding this comment

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

@garrettngarcia Thanks for looking into this highly annoying bug. However, I don't think that this is the correct way to fix this one.

The problem, as far as I understand it, is that we've got some over-zealous validation code. If you delete the player count from one of the boxes it sees the blank as zero, which is less than the minimum player could. So it helpfully re-populates the box with the minimum player count.

This needs to be redone so that the blank doesn't trigger adding the minimum player count. And we'd probably need to make sure that the form isn't submitted with an invalid player count – I think this validation code was added to prevent games being created with invalid counts.

@garrettngarcia
Copy link
Author

@ollybh I see. I had it that way in an earlier version, but didn't want games to be created with invalid min and max so came up with the selection solution. I can change it back.

Where should this form validation be done? In JS or in the back end? Can you point me to some current validation logic to get me started?

@ollybh
Copy link
Collaborator

ollybh commented Feb 24, 2025

Where should this form validation be done? In JS or in the back end? Can you point me to some current validation logic to get me started?

Ideally both. There does appear to already be some front end validation:

if game_params[:min_players].to_i < @min_p[title] || game_params[:max_players].to_i > @max_p[title]
return store(:flash_opts,
'Invalid playercount')
end

I'm not sure how much back end validation there is. It might go in Lib::Engine::Game::Base.initialize. There is sort of a check for player count in check_optional_rules!

def check_optional_rules!
min_players = @players.size
max_players = @players.size
error_string = meta.check_options(@optional_rules, min_players, max_players)&.[](:error)
raise OptionError, error_string if error_string
end

…but the default implementation of meta.check_options does nothing:

def check_options(_options, _min_players, _max_players); end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front page Issues relating front page, login, game creation etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mobile: iOS/android] setting playercount gets overriden by helper logic
2 participants