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

Remove the implementation of showing the old QR code #6847

Closed
wants to merge 3 commits into from

Conversation

Sachin-Mamoru
Copy link
Contributor

Purpose

Previous UI

image

New UI

image

With the new suggested change, we’ll be using refresh icon to generate the new qr code and won’t be showing the old qr code now. This behaviour is similar to backup code generation as well.

Checklist

  • e2e cypress tests locally verified. (for internal contributers)
  • Manual test round performed and verified.
  • UX/UI review done on the final implementation.
  • Documentation provided. (Add links if there are any)
  • Relevant backend changes deployed and verified
  • Unit tests provided. (Add links if there are any)
  • Integration tests provided. (Add links if there are any)

Security checks

@Achintha444
Copy link
Contributor

Design Review

  • Do we need to have the yellow box in the modal ? IMO it is not required, the modal is enough.
  • Did we get the content in the modal reviewed by the content team ?

closeOnDimmerClick={ false }
dimmer="blurring"
>
<Modal.Content data-testid={ `${testId}-regenerate-confirm-modal-content` }>
Copy link
Contributor

Choose a reason for hiding this comment

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

refrain from using data-testid use data-componentid instead.

Copy link
Contributor

@Achintha444 Achintha444 Sep 2, 2024

Choose a reason for hiding this comment

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

check other places and remove data-testid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a component we had earlier, not a new one. So it has already used testids and I think if we change them, a lot of e2e might break.

Copy link
Contributor

Choose a reason for hiding this comment

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

better to add a data-componentid param then, keeping the data-testid, and create an issue to check the E2E on this

…enticators/totp-authenticator.tsx

Co-authored-by: Achintha Isuru <[email protected]>
@wso2-jenkins-bot
Copy link
Contributor

🦋 Changeset detected

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

</Message.Content>
</Message>
{
commonConfig.accountSecurityPage.mfa.totp.showRegenerateConfirmation
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the showRegenerateConfirmation from config files as well. With the improved UI this config won't be used.

showRegenerateConfirmation: false

After the unification the config is always set to false. Therefore no point of keeping it at all.

<Message className="display-flex" size="small" warning>
<Icon name="warning sign" color="orange" corner />
<Message.Content className="tiny">
{ t(commonConfig.accountSecurityPage.mfa.totp.regenerateWarning) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Check the possibility of removing regenerateWarning config as well. Seems the usage has been removed this PR.

@Sachin-Mamoru
Copy link
Contributor Author

This will be addressed via a different approach, hence closing the PR.

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.

4 participants