-
Notifications
You must be signed in to change notification settings - Fork 55
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
Results page and PXP support for Hepatitis C #8249
Conversation
b92a987
to
59ebd9d
Compare
LGTM! Ty for all your work. |
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.
hahah I thought I was losing my mind seeing all the same changes that I thought I already reviewed in the previous PR. 😵 Didn't realize this was not up to date with main
😅 so reviewed the diff locally and codewise looks good!
Testing on dev4 also worked as expected! Nice work on this, Mike! 🎉
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.
Tested on dev 4 and looks great -- thanks Mike!
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.
changes look good but can you please rebase from main next time before opening a PR? this PR is really hard to read
also curious if git is smart enough to not apply the same changes again in the second merge
frontend/src/lang/en.ts
Outdated
h1: "For Hepatitis-C:", | ||
positive: { | ||
p0: "If you have a positive result, you will need a follow-up test to confirm your results. The organization that provided your test should be able to answer questions and provide referrals for follow-up testing.", | ||
p1: "<0>Visit the CDC website to learn more about a positive Hepatitis-C result.</0> (cdc.gov/hepatitis-c/testing/index.html#cdc_testing_results-testing-results).", |
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.
I know this is so minor but I don't think we should have a period between the link text and the URL in parentheses, so that we can match the copy for the other diseases. maybe this can be fixed in #8255
const mockDiseaseEnabledFlag = ( | ||
diseaseName: string, | ||
skipLowercase: boolean = false | ||
) => | ||
jest | ||
.spyOn(flaggedMock, "useFeature") | ||
.mockImplementation((flagName: string) => { | ||
// to handle casing of Hepatitis-C as hepatitisC | ||
if (skipLowercase) { | ||
return flagName === `${diseaseName}Enabled`; | ||
} | ||
return flagName === `${diseaseName.toLowerCase()}Enabled`; | ||
}); |
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.
this is a nit but I don't think this method needs the added complexity of a skipLowercase
parameter. I think we can just call it with the disease names in lower case
1c219c1
aa2ac27
to
1c219c1
Compare
Quality Gate passedIssues Measures |
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.
Looks good! Thanks for addressing Merethe's nits!
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.
🚢 thank you!
FRONTEND PULL REQUEST
Related Issue
Changes Proposed
Additional Information
Testing
Screenshots / Demos
Hepatitis-C already available as a selectable condition on Results page
Hepatitis-C already available as a condition in the Download Results spreadsheet
Hepatitis-C on Print Details modal
Spanish translations for Hepatitis-C on Print Details modal
Hepatitis-C and AOE responses on View Details modal
Hepatitis-C result on the PXP View Result
Spanish translations for Hepatitis-C result on the PXP View Result
Negative Hepatitis-C result with result guidance still shown