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.

Sign up for GitHub

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

Jump to bottom

feat(core): reset password #638

Merged
merged 1 commit into from
Mar 26, 2024
Merged

feat(core): reset password #638

merged 1 commit into from
Mar 26, 2024

Conversation

bc-alexsaiannyi
Copy link
Contributor

What/Why?

This PR adds functionality to reset password for Customer and includes 2 steps:

  • reset password email,
  • change password

it's based on PR with mutations.

Testing

locally

@bc-alexsaiannyi bc-alexsaiannyi requested a review from a team as a code owner March 12, 2024 08:53
@vercel Vercel
Copy link

vercel bot commented Mar 12, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
catalyst-latest ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 26, 2024 8:21pm
8 Ignored Deployments
Name Status Preview Comments Updated (UTC)
catalyst ⬜️ Ignored (Inspect) Mar 26, 2024 8:21pm
catalyst-1millionproducts-store ⬜️ Ignored (Inspect) Visit Preview Mar 26, 2024 8:21pm
catalyst-2000ms-api ⬜️ Ignored (Inspect) Mar 26, 2024 8:21pm
catalyst-au ⬜️ Ignored (Inspect) Visit Preview Mar 26, 2024 8:21pm
catalyst-no-kv ⬜️ Ignored (Inspect) Mar 26, 2024 8:21pm
catalyst-nodejs ⬜️ Ignored (Inspect) Mar 26, 2024 8:21pm
catalyst-storybook ⬜️ Ignored (Inspect) Visit Preview Mar 26, 2024 8:21pm
catalyst-uk ⬜️ Ignored (Inspect) Visit Preview Mar 26, 2024 8:21pm

@bc-alexsaiannyi bc-alexsaiannyi added the draft Ready for initial review label Mar 12, 2024
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Mar 13, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 1093c67

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

This PR includes changesets to release 3 packages
Name Type
@bigcommerce/catalyst-core Minor
@bigcommerce/components Patch
@bigcommerce/docs Patch

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

Click here if you're a maintainer who wants to add another changeset to this PR

@matthewvolk
Copy link
Contributor

Needs changeset 🦋

@bc-alexsaiannyi bc-alexsaiannyi removed the draft Ready for initial review label Mar 14, 2024
@bc-alexsaiannyi bc-alexsaiannyi force-pushed the reset-password branch 2 times, most recently from e9ed37d to 0517143 Compare March 18, 2024 14:16
BC-krasnoshapka
BC-krasnoshapka reviewed Mar 20, 2024
View reviewed changes
apps/core/app/[locale]/(default)/login/_components/reset-password-form.tsx
Comment on lines +39 to +56
<Button
className="relative w-fit items-center px-8 py-2"
data-button
disabled={pending}
variant="primary"
>
<>
{pending && (
<>
<span className="absolute z-10 flex h-full w-full items-center justify-center bg-gray-400">
<Spinner aria-hidden="true" className="animate-spin" />
</span>
<span className="sr-only">{t('spinnerText')}</span>
</>
)}
<span aria-hidden={pending}>{t('submitText')}</span>
</>
</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed the same component is used in #665 change password form.

Copy link
Contributor Author

@bc-alexsaiannyi bc-alexsaiannyi Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah. Actually, now we have around 6 forms with submit button which are different basically only with their texts. I was thinking about creating abstraction with text params that will return <SubmitButton> & reuse it across all forms💅 That why for now I decide to keep it separately.

jorgemoya
jorgemoya approved these changes Mar 25, 2024
View reviewed changes
Copy link
Contributor

@jorgemoya jorgemoya left a comment

Choose a reason for hiding this comment

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

Looks good but we do need to know if we want to have a different path to reset passwords other than reusing /login 🤔

@bc-alexsaiannyi
Copy link
Contributor Author

Looks good but we do need to know if we want to have a different path to reset passwords other than reusing /login 🤔

Yeah, it's a good point, Jorge. Now, when RequestResetPasswordInput can deal with path, I've added it to submit function also. I believe it should be done explicitly since if we skip this argument on calling submit fn we get next result - https://your-store/login.php?c=XX&t=XXX. In this way it inherits || reproduces current stencil behaviour, if we miss completely path in arguments, we get smth like https://your-store?c=XX&t=XXX.
So to receive https://your-store?login?c=XX&t=XXX I'm providing it as an argument:

submitResetPasswordForm({ formData, reCaptchaToken, path: '/login' });

yurytut1993
yurytut1993 reviewed Mar 26, 2024
View reviewed changes
apps/core/app/[locale]/(default)/login/_actions/submit-reset-password-form.ts Outdated
}

return {
status: 'failed',
Copy link
Contributor

Choose a reason for hiding this comment

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

should we change status to error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I was sure that I did change it. Thanks for catching this.

yurytut1993
yurytut1993 approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@yurytut1993 yurytut1993 left a comment

Choose a reason for hiding this comment

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

👌

@github-actions GitHub Actions
Copy link
Contributor

⚡️🏠 Lighthouse report

We ran Lighthouse against the changes and produced this report. Here's the summary:

Category Score
🟢 Performance 99
🟢 Accessibility 100
🟠 Best practices 78
🟢 SEO 92

Lighthouse ran against https://catalyst-latest-llo8m97m6-bigcommerce-platform.vercel.app/

@bc-alexsaiannyi bc-alexsaiannyi added this pull request to the merge queue Mar 26, 2024
Merged via the queue into main with commit a1f7970 Mar 26, 2024
11 checks passed
@bc-alexsaiannyi bc-alexsaiannyi deleted the reset-password branch March 26, 2024 20:25
This was referenced Mar 26, 2024
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BC-krasnoshapka BC-krasnoshapka BC-krasnoshapka left review comments

@jorgemoya jorgemoya jorgemoya approved these changes

@yurytut1993 yurytut1993 yurytut1993 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bc-alexsaiannyi @matthewvolk @jorgemoya @yurytut1993 @BC-krasnoshapka