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

#3087/#3099: Quick Content Fixes - [MS] #3244

Merged
merged 5 commits into from
Dec 30, 2024
Merged

Conversation

Matt-Spence
Copy link
Contributor

Ticket

Resolves #3087
Resolves #3099

Changes

  • Ensure error messages display in messages when adding domain managers
  • remove duplicate sentences from senior official page when no senior official is found

Context for reviewers

Setup

Code Review Verification Steps

Put in an invalid email address when adding a domain manager -> check that the correct error message displays (including at the top of the page and on the input itself)

View senior officials in org and non-org mode for domains that don't have senior officials listed. Check that duplicate sentences have been removed.

As the original developer, I have

Satisfied acceptance criteria and met development standards

  • Met the acceptance criteria, or will meet them in a subsequent PR
  • Created/modified automated tests
  • Update documentation in READMEs and/or onboarding guide

Ensured code standards are met (Original Developer)

  • If any updated dependencies on Pipfile, also update dependencies in requirements.txt.
  • Interactions with external systems are wrapped in try/except
  • Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

  • Tag @dotgov-designers in this PR's Reviewers for design review. If code is not user-facing, delete design reviewer checklist
  • Verify new pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
  • Checked keyboard navigability
  • Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

  • Pulled this branch locally and tested it
  • Verified code meets all checks above. Address any checks that are not satisfied
  • Reviewed this code and left comments. Indicate if comments must be addressed before code is merged
  • Checked that all code is adequately covered by tests
  • Verify migrations are valid and do not conflict with existing migrations

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.

  • New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
  • Checked keyboard navigability
  • Meets all designs and user flows provided by design/product
  • Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
  • (Rarely needed) Tested as both an analyst and applicant user

As a designer reviewer, I have

Verified that the changes match the design intention

  • Checked that the design translated visually
  • Checked behavior. Comment any found issues or broken flows.
  • Checked different states (empty, one, some, error)
  • Checked for landmarks, page heading structure, and links

Validated user-facing changes as a designer

  • Checked keyboard navigability
  • Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
  • Tested with multiple browsers (check off which ones were used)
    • Chrome
    • Microsoft Edge
    • FireFox
    • Safari
  • (Rarely needed) Tested as both an analyst and applicant user

References

Screenshots

Copy link

🥳 Successfully deployed to developer sandbox ms.

@@ -37,6 +37,9 @@
</nav>
{% endif %}
{% endblock breadcrumb %}

{% include "includes/form_errors.html" with form=form %}

Copy link
Contributor

Choose a reason for hiding this comment

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

I read on the ticket that the error is supposed to appear on the field level and at the top of the page. Was there an update that wasn't noted? I don't get the error at the top

Screenshot 2024-12-23 at 10 24 24 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't deployed to MS at the time you did the review, sorry. I have two PR's up but forgot that I only have one sandbox 🫠. Should deploy shortly.

@Matt-Spence Matt-Spence requested a review from a team December 23, 2024 16:27
Copy link
Contributor

@asaki222 asaki222 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

🥳 Successfully deployed to developer sandbox ms.

@kristinacyin kristinacyin requested review from Katherine-Osos and kristinacyin and removed request for a team December 23, 2024 18:54
Copy link
Contributor

@kristinacyin kristinacyin left a comment

Choose a reason for hiding this comment

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

A couple of things I noticed:

  1. Multiple success messages: There's two success alerts after I correct an erroneous zip code in the Organization page (org model). Noticed that same thing happens for other fields as well on the page.
    image

  2. Placement of page error alert:

Usually page errors are placed above the heading like so:
image

However, the page error alerts (for all fields) on the Organization page are below the heading, under the instructional paragraph. Was this intentional? Can we move them above so that they're more consistent?
getgov-ms app cloud gov_organization_

@Matt-Spence
Copy link
Contributor Author

A couple of things I noticed:

  1. Multiple success messages: There's two success alerts after I correct an erroneous zip code in the Organization page (org model). Noticed that same thing happens for other fields as well on the page.
    image
  2. Placement of page error alert:

Usually page errors are placed above the heading like so: image

However, the page error alerts (for all fields) on the Organization page are below the heading, under the instructional paragraph. Was this intentional? Can we move them above so that they're more consistent? getgov-ms app cloud gov_organization_

I think these are all on main 🫠 I didn't change anything on the organization or security email pages!

Copy link

🥳 Successfully deployed to developer sandbox ms.

@kristinacyin kristinacyin self-requested a review December 30, 2024 19:10
Copy link
Contributor

@kristinacyin kristinacyin left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the additional fix!

@Matt-Spence Matt-Spence merged commit bc142e4 into main Dec 30, 2024
8 of 9 checks passed
@Matt-Spence Matt-Spence deleted the ms/3087-quick-fixes branch December 30, 2024 21:31
Copy link

🥳 Successfully deployed to developer sandbox ms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants