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

[74] Enter Evaluators A11y Fix #343

Merged
merged 30 commits into from
Jan 16, 2025
Merged

[74] Enter Evaluators A11y Fix #343

merged 30 commits into from
Jan 16, 2025

Conversation

emmabjj
Copy link
Contributor

@emmabjj emmabjj commented Jan 3, 2025

Related ticket: #200 #74
Figma

Changes made in this PR:

  • Given an existing user who was created without a first and last name,
    when adding that user as an evaluator to a challenge phase,
    then require and update the user with the provided first and last name.
  • Update the UI to use a single field for the first & last name instead of two separate form fields.
  • Update the UI to remove the subtitle below the form fields.
  • Add inline error messages and use usa-alert for form validation.
  • Update the challenge phase title.
  • Make alt text for "Add Evaluator" decorative

Updated UI

Screenshot 2025-01-03 at 1 01 29 PM

Validation errors

Empty form

Screenshot 2025-01-03 at 1 10 52 PM

Only a first name

Screenshot 2025-01-03 at 11 08 35 AM

Missing last name

Screenshot 2025-01-03 at 11 08 46 AM

User with invalid role

Screenshot 2025-01-03 at 11 07 05 AM

Required first and last name for existing user

Screenshot 2025-01-03 at 11 43 49 AM

Existing user without first and last name updated with name entered

Screenshot 2025-01-03 at 11 46 45 AM

Mobile

Screenshot 2025-01-03 at 1 11 18 PM

WAVE

Screenshot 2025-01-03 at 1 13 13 PM

@emmabjj emmabjj self-assigned this Jan 3, 2025
@emmabjj emmabjj requested a review from stepchud January 3, 2025 19:14
@r-bartlett-gsa r-bartlett-gsa added this to the Sprint 01/14/25 milestone Jan 3, 2025
@r-bartlett-gsa r-bartlett-gsa linked an issue Jan 3, 2025 that may be closed by this pull request
67 tasks
@emmabjj emmabjj requested a review from stepchud January 8, 2025 19:06
Copy link
Contributor

@stepchud stepchud left a comment

Choose a reason for hiding this comment

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

The missing inline error message is probably a blocker given the requirements. Plus some other suggestions to improve the design.

app/controllers/evaluators_controller.rb Show resolved Hide resolved
app/services/evaluator_management_service.rb Outdated Show resolved Hide resolved
@emmabjj emmabjj requested a review from stepchud January 15, 2025 01:25
def add_existing_user_as_evaluator(user)
return user_already_added(user) if @phase.evaluators.include?(user)
return invalid_role(user) unless User::VALID_EVALUATOR_ROLES.include?(user.role)

if @invitation_params[:full_name].present?
Copy link
Contributor

@stepchud stepchud Jan 15, 2025

Choose a reason for hiding this comment

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

I thought full_name param was required, why is it optional here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you're right - now that full_name is required and used in the validations, I don't need that check. Some of the tests weren't including the full_name. I removed that check and updated the tests. Should be good to go, thx! 👍

@stepchud stepchud dismissed their stale review January 15, 2025 23:01

resolved

@emmabjj emmabjj requested a review from stepchud January 16, 2025 17:32
Copy link
Contributor

@stepchud stepchud left a comment

Choose a reason for hiding this comment

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

Looks great 😍

@emmabjj emmabjj merged commit eb7d1b5 into dev Jan 16, 2025
11 checks passed
@emmabjj emmabjj deleted the 74_enter_evaluators_a11y_fix branch January 16, 2025 18:12
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.

Enter Evaluators
3 participants