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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

LG-12824: camera perm msg #10379

Merged
merged 5 commits into from
May 1, 2024
Merged

LG-12824: camera perm msg #10379

merged 5 commits into from
May 1, 2024

Conversation

dawei-nava
Copy link
Contributor

@dawei-nava dawei-nava commented Apr 8, 2024

馃帿 Ticket

Link to the relevant ticket:
LG-12824

馃洜 Summary of changes

Update the camera permission message.

Refactor form step error message processing(Alert), add a messageProcessor to format/process the configured message.

馃摐 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Step 1: Login with or without selfie on mobile
  • Step 2: Proceed to document capture/upload
  • Step 3: Deny camera access
  • Step 4: Verify the error message

馃憖 Screenshots

If relevant, include a screenshot or screen capture of the changes.

After:
En Es Fr Zh
Permission_En Permission_Es CamPerm_Fr CameraPermZh

@dawei-nava dawei-nava marked this pull request as ready for review April 10, 2024 18:03
Copy link

@kellular kellular left a comment

Choose a reason for hiding this comment

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

Cross-referenced with the DOS translations. LGTM!

Also confirming that I included screenshots from this PR in the LQA doc for DOS to review. Thanks Dawei!

Copy link
Contributor

@charleyf charleyf left a comment

Choose a reason for hiding this comment

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

I'm approving because this does work. I'm including some changes I think will improve future readability in comments. I think we should avoid passing messagProcessor and use a pattern more like InPersonOutageAlert.

@@ -218,6 +219,17 @@ function getFieldActiveErrorFieldElement(
}
}

export function StepErrorAlert({ error }: { error: Error }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect this component to look more like InPersonOutageAlert and HybridDocCaptureWarning. Practically, that would look like removeing the messageProcessor argument from form-error and instead moving the transformation code inline like this (this code may not work as is).

export function StepErrorAlert({ error }: { error: Error }) {
  const { message } = error;
  const messageProcessor = formatHTML(message, {
    strong: ({ children }) => <strong>{children}</strong>,
    span: ({ children }) => <span className="display-block margin-top-1em">{children}</span>,
  });
  const transformedMessage = messageProcessor(message);

  return (
    <Alert key={message} type="error" className="margin-bottom-4">
      { !!transformedMessage ? transformedMessage : message }
    </Alert>
  );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charleyf , unfortunately, this is a shared component form-steps, most of cases, this transformation not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I missed how this component is shared. I do think this code could be changed (or commented) so that it's clearer that we only need and expect the messageProcessor for the single type of error it's needed for (a CameraAccessDeclinedError with this.isDetail === true), but it works as is too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charleyf, since we do the transformation logic in form-steps, maybe it is more natural to add it in form-error other than CameraAccessDeclinedError.

Copy link
Contributor

Choose a reason for hiding this comment

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

That could help. What about in CameraAccessDeclinedError also do the message processing. This allows us to make no changes in either form-error or form-steps. So something like this:

    return this.isDetail
      ? messageProcessor(t('doc_auth.errors.camera.blocked_detail_html', {
          app_name: getConfigValue('appName'),
        }))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charleyf , i believe that's the first thing i tried? For some reason if you pass a html string to the <Alert> component as Error.message(which has type of a string), it's not rendering as html, but as a string as is. There are may be some small tweak can be done to it. What's your thought?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this does the same thing, but without modifying any of the more general files:
https://github.com/18F/identity-idp/compare/charley/suggestions-for-camera-perms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again this try format message every time which mostly not true or have different styling requirements?

{stepErrors.map((error) => {
        const formattedMessage = formatMessage(error.message);
  return (
    <Alert key={error.message} type="error" className="margin-bottom-4">
      {formattedMessage || error.message}
    </Alert>
  );
})}

I am wondering what is the clean way to render error.message correctly in Alert if it contains html fragment.

Copy link
Contributor

@charleyf charleyf Apr 12, 2024

Choose a reason for hiding this comment

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

what is the clean way to render error.message correctly in Alert if it contains html fragment

I'm not sure there is a good way to do this. You're trying to detect when something is a string vs when it has HTML tags, which seems like a hard problem.

I'm proposing that instead of trying to detect HTML vs strings, make the HTML formatter function work with either. That avoids a lot of complexity.

@dawei-nava dawei-nava force-pushed the dwang/LG-12824_camera_perm_msg branch from 7686ea1 to e727a79 Compare April 30, 2024 15:46
@dawei-nava dawei-nava force-pushed the dwang/LG-12824_camera_perm_msg branch from 135f7d4 to c9d12c4 Compare April 30, 2024 22:40
@dawei-nava dawei-nava merged commit a5c23dc into main May 1, 2024
2 checks passed
@dawei-nava dawei-nava deleted the dwang/LG-12824_camera_perm_msg branch May 1, 2024 00:46
samathad2023 pushed a commit that referenced this pull request May 11, 2024
* LG-12824: Update camera permission message.

changelog: User-Facing Improvements, Doc Auth, Camera permission messages.

* LG-12824: Update with DOS translations.

* LG-12824: minor refactor on how to pass the processor.

* LG-12824: French translation has app name.

* LG-12824: zh translation.
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

4 participants