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

Add forgot password component #104

Merged
merged 17 commits into from
Oct 15, 2024
Merged

Add forgot password component #104

merged 17 commits into from
Oct 15, 2024

Conversation

MaHaWo
Copy link
Collaborator

@MaHaWo MaHaWo commented Oct 14, 2024

  • add classical 'forgot password' page
  • try to include backend API calls
  • add localization: de, en

@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 51 lines in your changes missing coverage. Please review.

Project coverage is 15.64%. Comparing base (d4ea8f8) to head (e7e098b).

Files with missing lines Patch % Lines
...ntend/src/lib/components/UserForgotPassword.svelte 0.00% 46 Missing and 1 partial ⚠️
...tend/src/lib/components/DataInput/DataInput.svelte 0.00% 2 Missing ⚠️
...tend/src/routes/userLand/lostPassword/+page.svelte 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #104      +/-   ##
==========================================
- Coverage   15.75%   15.64%   -0.12%     
==========================================
  Files          94       95       +1     
  Lines        4538     4571      +33     
  Branches      122      123       +1     
==========================================
  Hits          715      715              
- Misses       3756     3788      +32     
- Partials       67       68       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MaHaWo MaHaWo requested a review from lkeegan October 14, 2024 12:56
Copy link
Member

@lkeegan lkeegan left a comment

Choose a reason for hiding this comment

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

lgtm - some minor suggestions

frontend/src/locales/de.json Outdated Show resolved Hide resolved
throwOnError: true
};

await resetForgotPassword(data)
Copy link
Member

Choose a reason for hiding this comment

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

I think it's not recommended to mix await and .then styles - can lead to confusion (see e.g. this random blog post https://maximorlov.com/why-you-shouldnt-mix-promise-then-with-async-await/)

Copy link
Collaborator Author

@MaHaWo MaHaWo Oct 15, 2024

Choose a reason for hiding this comment

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

ok. With respect to the linked blog post I don´t think it's a problem in this specific case because it has a catch block at the end, but as a general rule it's certainly valid...


const maildata = {
component: Input,
type: 'text',
Copy link
Member

Choose a reason for hiding this comment

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

if you make the type 'email' and make it required then you get validation of the email automatically: see e.g. https://github.com/ssciwr/mondey/blob/main/frontend/src/lib/components/Admin/Login.svelte#L24-L31

Comment on lines 73 to 87
<DataInput
component={maildata.component}
bind:value={maildata.value}
properties={maildata.props}
/>
</div>

<div class="m-2 flex w-full items-center justify-center p-2">
<Button
size="md"
on:click={(event) => {
valid = validateEmail(maildata.value);
showAlert = !valid;
if (valid) {
submitData(maildata.value);
Copy link
Member

Choose a reason for hiding this comment

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

If you wrap the inputs in a form and make the button of type "submit" then the user pressing enter will press the button, e.g.

<form onsubmit={preventDefault(doLogin)}>

<Button type="submit" class="mb-6">Login</Button>

@MaHaWo
Copy link
Collaborator Author

MaHaWo commented Oct 15, 2024

ok, I made the changes you remarked. you want to have another look @lkeegan or should I merge this?

@lkeegan
Copy link
Member

lkeegan commented Oct 15, 2024

ok, I made the changes you remarked. you want to have another look @lkeegan or should I merge this?

thanks! yes please go ahead - then I'll rebase #107

Copy link

@MaHaWo MaHaWo merged commit 1fbb03d into main Oct 15, 2024
5 checks passed
@MaHaWo MaHaWo deleted the add-forgot-password-component branch October 15, 2024 09:27
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.

3 participants