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

Spike binny in app feedback #10166

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

Conversation

dumathane
Copy link
Contributor

Description of Change

Screenshots/Video

Testing

  • Tested on iOS
  • Tested on Android

Reviewer Validations

PR Checklist

Reviewer: Confirm the items below as you review

  • PR is connected to issue(s)
  • Tests are included to cover this change (when possible)
  • No magic strings (All string unions follow the Union -> Constant type pattern)
  • No secrets or API keys are checked in
  • All imports are absolute (no relative imports)
  • New functions and Redux work have proper TSDoc annotations

For QA

Run a build for this branch

// satisfaction,
// },
// }
// },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committing these analytics commented out for the other ticket to provide.

</Box>
<Box
borderTopWidth={theme.dimensions.borderWidth}
borderColor={theme.colors.border.prescriptionDivider as BorderColorVariant}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this color be renamed if it's used in multiple places?

"inAppFeedback.popup.title": "Leave feedback?",
"inAppFeedback.popup.body": "Leaving us feedback helps us improve.",
"inAppFeedback.popup.notNow": "Not now",
"inAppFeedback.popup.accept": "Give feedback",
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding this translation twice, you could add it once with giveFeedback as the key

"inAppFeedback.a11yHint": "Go to the give feedback page",
"inAppFeedback.popup.title": "Leave feedback?",
"inAppFeedback.popup.body": "Leaving us feedback helps us improve.",
"inAppFeedback.popup.notNow": "Not now",
Copy link
Contributor

Choose a reason for hiding this comment

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

This could just use 'notNow' as the key

"inAppFeedback.legalReqs.number": "OMB Number: 2900-0876",
"inAppFeedback.legalReqs.expiration": "Expiration: 02/28/2026",
"inAppFeedback.legalReqs.burdenTime": "Burden Time: 3 Minutes",
"inAppFeedback.legalReqs.paragraph": "VA may utilize individual Veteran survey data from this survey or other sources to ensure the final scores truly and accurately represent the experiences of Veterans. This information is collected in accordance with section 3507 of the Paperwork Reduction Act of 1995. Title 38, United States Code, allows us to ask for this information. We estimate that you will need an average of 3 minutes to review the instructions and complete this survey. The results of this survey will be used to inform opportunities for program improvement in the quality of VA services. Participation in this survey is voluntary, and your decision not to respond will have no impact on VA benefits or services which you may currently be receiving. VA cannot conduct or sponsor a collection of information unless a valid OMB control number is displayed. You are not required to respond to a collection of information if this number is not displayed. Valid OMB control numbers can be located on the OMB Internet Page at https://www.reginfo.gov/public/do/PRAMain. Information gathered will be kept private to the extent provided by law.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the link be clickable?

isTextArea={true}
value={task}
testID="AppFeedbackTaskID"
onChange={(val) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably simplify to onChange={setTaskOverride}


const radioGroupProps: RadioGroupProps<string> = {
isRadioList: false,
onChange: onChange,
Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably just be onChange: setSatisfaction

@@ -257,6 +257,28 @@ export function useExternalLink(): (url: string, eventParams?: EventParams) => v
}
}

export function useGiveFeedback(): (task: string) => void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider creating a constant to collect all the possible values in one place

@@ -202,6 +208,9 @@ function DeveloperScreen({ navigation }: DeveloperScreenSettingsScreenProps) {
</TextView>
</Box>
</TextArea>
<TextArea>
<Button onPress={onFeedback} label={'In App Feedback Screen'} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a translation?

@github-actions github-actions bot added the FE-Changes Requested Requested changes from Eng or QA label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE-Changes Requested Requested changes from Eng or QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants