-
Notifications
You must be signed in to change notification settings - Fork 4
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
2133 tribe discoverability #2999
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2999 +/- ##
===========================================
+ Coverage 93.05% 93.10% +0.05%
===========================================
Files 276 275 -1
Lines 7108 7047 -61
Branches 599 601 +2
===========================================
- Hits 6614 6561 -53
+ Misses 399 392 -7
+ Partials 95 94 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -171,7 +177,52 @@ function Home() { | |||
handleChange={handleChange} | |||
handleBlur={handleBlur} | |||
/> | |||
{shouldShowSttComboBox && ( | |||
<div className="usa-form-group"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im no frontend guy so take my question with a grain of salt! Should this get moved into it's own component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep! it absolutely should. there are a few idiosyncrasies with the radio input behavior that made me feel un-confident moving it over to its own component, but i captured that task and open question in #3015
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One general question but works as advertised!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reitermb @ttran-hub do you all have time to add this change to the PR with the release notes? Please note the caveat @jtimpe mentions in the summary. This PR will be approved today. |
@@ -171,7 +177,52 @@ function Home() { | |||
handleChange={handleChange} | |||
handleBlur={handleBlur} | |||
/> | |||
{shouldShowSttComboBox && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtimpe looks like we are undoing the work implemented here to display the STT combobox only for those who sign-in via login.gov. When ACF users request access they do not see the combobox because they are associated with the federal government. in the screenshot below s=staging, q = qasp. i included the email that acf user receives which reflects the missing STT option (this is to be addressed in a future ticket).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, thank you. i added the conditional around the Jurisdiction Type selection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtimpe @reitermb summary of review below:
- STT request access screen appears as expected ✔️
- request access emails for STTs appear as expected ✔️
- ACF request access screen appears as expected
⚠️ - request access emails for ACF user appear as expected (to-do in future ticket) 📋
- TDP Knowledge Center updated to reflect change (to-do in KC ticket) 📋
Is this what you had in mind? Threw together a quick draft Improved the usability of TDP's request access form First-time users to TDP provide their first name, last name, and associated state, tribe, or territory to help OFA administrators confirm that they should be granted access to the system. We've enhanced the form by separating the selection of whether you work at a state, tribe, or territory from the selection of the name of that jurisdiction to eliminate confusion about whether tribes need to select the name of their tribal program or the name of the state it's located in. |
@ADPennington Also noting that we'll throw an updated screenshot / content onto the request access steps of our getting started in TDP guides. |
@reitermb made some minor revisions above. recommend taking this conversation to the relevant KC ticket or PR. thank you! |
@jtimpe i ran into a migration issue on this PR. |
@ADPennington it was failing to connect to the db for some reason - could have been a broken ssh tunnel or maintenance issue, i'm not sure. but i re-ran and it made it past the migration step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
acf request form looks good! thanks @jtimpe 🚀
Summary of Changes
Pull request closes #2133
How to Test
Initial
access request state and has no location or roles assigned./home
and fill out the request access formDeliverables
Checklist of ACs:
lfrohlich
and/oradpennington
confirmed that ACs are met.Deliverable 2: Tested Code
CodeCov Report
comment in PR)CodeCov Report
comment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjolly
andttran-hub
using Accessibility Insights reveal any errors introduced in this PR?Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
Deliverable 8: User Research
Research product(s) clearly articulate(s):