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

Sc-17544: Clear form refactoring #1037

Merged
merged 25 commits into from
May 30, 2023
Merged

Sc-17544: Clear form refactoring #1037

merged 25 commits into from
May 30, 2023

Conversation

masskoder
Copy link
Collaborator

@masskoder masskoder commented May 30, 2023

Scope of changes

This adds clear form refactoring.

Type of change

  • bug fix
  • new feature
  • documentation
  • other (refactor)

Acceptance criteria

Describe how reviewers can test this change to be sure that it works correctly. Add a checklist if possible

Author checklist

  • I have manually tested the change and/or added automation in the form of unit tests or integration tests
  • I have updated the dependencies list
  • I have recompiled and included new protocol buffers to reflect changes I made
  • I have added new test fixtures as needed to support added tests
  • Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)
  • I have moved the associated Shortcut story to "Ready for Review"

Reviewer(s) checklist

  • Any new user-facing content that has been added for this PR has been QA'ed to ensure correct grammar, spelling, and understandability.

@masskoder masskoder requested a review from daniellemaxwell May 30, 2023 14:38
@masskoder masskoder changed the title Sc-17544 Sc-17544: Clear form refactoring May 30, 2023
Copy link
Collaborator

@daniellemaxwell daniellemaxwell left a comment

Choose a reason for hiding this comment

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

I've given this an initial pass and this is a good start. I'll take another look when the changes we talked about are ready.

onResetModalClose={handleResetClick}
isOpened={isOpen}
handleResetForm={handleResetForm}
resetFormType="basic"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
resetFormType="basic"
resetFormType={StepEnum.CONTACTS}

</Trans>
{isResetAllType ? (
<Trans>
Click “Reset” to clear and reset the registration form. All data will be
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Click Reset to clear and reset the registration form. All data will be
Click "Reset" to clear and
<Text as="span" fontWeight={'bold'}>
{' '}
reset the form{' '}
</Text>{' '}. All data will be

Copy link
Collaborator

Choose a reason for hiding this comment

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

{isResetAllType ? (
<Trans>
Click “Reset” to clear and reset the registration form. All data will be
deleted and you will be re-directed to the beginning of the form and you will
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
deleted and you will be re-directed to the beginning of the form and you will
deleted and cleared from the registration form. You may want to export the data first.

<Trans>
Click “Reset” to clear and reset the registration form. All data will be
deleted and you will be re-directed to the beginning of the form and you will
be required to restart the registration process
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
be required to restart the registration process
After clearing, you will be taken to the start of the form.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not currently taken to the start of the form after clearing. Will this be added or should the copy be updated?

@@ -88,9 +92,9 @@ const ConfirmationResetForm = (props: any) => {
{' '}
this section{' '}
</Text>{' '}
of the registration form. All data will be cleared from only this section of
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes were made to the Clear & Reset Section modal, but they should be in the Clear & Reset Form modal.
Screenshot 2023-05-30 at 1 03 34 PM

Copy link
Collaborator

@daniellemaxwell daniellemaxwell left a comment

Choose a reason for hiding this comment

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

Thanks for making changes quickly! If you could double check the copy in the clear section and clear form modals before merging, that would be great. The rest looks good.

<Text as="span" fontWeight={'bold'}>
reset the form.
</Text>{' '}
reset the registration form. All data will be deleted and you will be
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
reset the registration form. All data will be deleted and you will be
All data will be deleted and cleared

reset the form.
</Text>{' '}
reset the registration form. All data will be deleted and you will be
re-directed to the beginning of the form and you will be required to restart
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
re-directed to the beginning of the form and you will be required to restart
from the registration form. You may want to export the data first.

</Text>{' '}
reset the registration form. All data will be deleted and you will be
re-directed to the beginning of the form and you will be required to restart
the registration process
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
the registration process
After clearing you will be taken to the start of the form.

{' '}
this section{' '}
</Text>{' '}
of the form. All data will be deleted and cleared from the registration form.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
of the form. All data will be deleted and cleared from the registration form.
of the registration form. All data will be cleared from only this section of the registration form.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These suggestions should make the Clear & Reset Section match the Figma file.

this section{' '}
</Text>{' '}
of the form. All data will be deleted and cleared from the registration form.
You may want to export the data first. After clearing, you will be taken to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
You may want to export the data first. After clearing, you will be taken to
Other sections of the registration form will not be cleared.

</Text>{' '}
of the form. All data will be deleted and cleared from the registration form.
You may want to export the data first. After clearing, you will be taken to
the start of the form.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
the start of the form.

<ModalOverlay />
<ModalContent width={'100%'}>
<ModalHeader data-testid="confirmation-modal-header" textAlign={'center'}>
<Trans id="Clear & Reset Registration Form">Clear & Reset Registration Form</Trans>
{isResetAllType ? (
<Trans>Clear & Reset Registration Form</Trans>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Trans>Clear & Reset Registration Form</Trans>
<Trans>Clear & Reset Form</Trans>

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is what I saw before the suggested changes to match the Figma file:
Screenshot 2023-05-30 at 1 13 07 PM

@masskoder masskoder merged commit 12f5667 into main May 30, 2023
@masskoder masskoder deleted the sc-17544 branch May 30, 2023 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants