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

Fixed scroll/focus logic on close of modal #28266

Merged
merged 13 commits into from Mar 4, 2024

Conversation

cosu419
Copy link
Contributor

@cosu419 cosu419 commented Feb 29, 2024

Summary

  • Updated scroll/focus behavior onClose of Report Modal
  • Set CSRF token from fetch representatives response headers
  • Added method for formatting request object into snake case

Related issue(s)

Related issue(s)

New unit test written for method that formats reports

@cosu419 cosu419 requested a review from a team as a code owner February 29, 2024 17:07
@va-vfs-bot va-vfs-bot temporarily deployed to master/co-022824-Find-a-Rep-csrf-token/main February 29, 2024 17:52 Inactive
@Andrew565
Copy link
Contributor

PR has failing required tests, opting not to review at this time. - Tier 2/FE COP

holdenhinkle
holdenhinkle previously approved these changes Mar 1, 2024
Copy link
Contributor

@holdenhinkle holdenhinkle left a comment

Choose a reason for hiding this comment

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

LGTM

@va-vfs-bot va-vfs-bot temporarily deployed to master/co-022824-Find-a-Rep-csrf-token/main March 1, 2024 17:14 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to master/co-022824-Find-a-Rep-csrf-token/main March 1, 2024 19:51 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to master/co-022824-Find-a-Rep-csrf-token/main March 4, 2024 01:03 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to master/co-022824-Find-a-Rep-csrf-token/main March 4, 2024 13:44 Inactive
Copy link

@va-vfs-bot va-vfs-bot left a comment

Choose a reason for hiding this comment

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

Sentry call found

Sentry captures a lot of data, and we want to make sure that we only keep information that will be useful for troubleshooting issues. This means that PII should not be recorded.

What you can do

Review your call to Sentry and see if you can reasonably reduce any information that is included, or wait for a VSP review.

holdenhinkle
holdenhinkle previously approved these changes Mar 4, 2024
Copy link
Contributor

@holdenhinkle holdenhinkle 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

@va-vfs-bot va-vfs-bot left a comment

Choose a reason for hiding this comment

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

ESLint is disabled

vets-website uses ESLint to help enforce code quality. In most situations we would like ESLint to remain enabled.

What you can do

See if the code can be refactored to avoid disabling ESLint, or wait for a VSP review.

@va-vfs-bot va-vfs-bot temporarily deployed to master/co-022824-Find-a-Rep-csrf-token/main March 4, 2024 15:53 Inactive
@holdenhinkle holdenhinkle self-requested a review March 4, 2024 16:32
Copy link
Contributor

@holdenhinkle holdenhinkle left a comment

Choose a reason for hiding this comment

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

LGTM

@cosu419 cosu419 merged commit 5a96985 into main Mar 4, 2024
81 of 82 checks passed
@cosu419 cosu419 deleted the co-022824-Find-a-Rep-csrf-token branch March 4, 2024 17:20
pjhill pushed a commit that referenced this pull request Mar 14, 2024
* Fixed scroll/focus logic on close of modal

* Updated feature toggle references to "find_a_representative_enabled"

* Refactored scroll/focus logic. Now focuses alert when reports increase

* Added new feature flag

* Setting csrfToken on successful fetch. Using apiRequest utility rather than fetch.

* Updated error verbiage

* Setting report object properties to snake case items for API

* Moved formatting logic to dedicated formatReportBody. Added unit test.
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

5 participants