Skip to content

Conversation

@mohitm15
Copy link
Contributor

Fixes #3876

Short description of what this resolves:

UI enhancement in forgot password window: Adding Password validation and toggle icon.

Changes proposed in this pull request:

  • reset-password.js
  • reset-password.hbs

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@snitin315
Copy link
Member

Please provide screenshots / GIF of changes.

@mohitm15
Copy link
Contributor Author

Please provide screenshots / GIF of changes.

I am unable to get the changes as I earlier mentioned. Please confirm this

Comment on lines 24 to 28
<div class="ui small basic icon buttons">
<button type="button" class="ui button" {{action 'showPassword' 'New'}}>
<i class="{{if showPassNew 'hide' 'unhide'}} basic icon"></i>
</button>
</div>
Copy link
Member

@snitin315 snitin315 Jan 28, 2020

Choose a reason for hiding this comment

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

correct indentation here.

Copy link
Contributor Author

@mohitm15 mohitm15 Jan 28, 2020

Choose a reason for hiding this comment

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

at line 24-28!

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I push commit now? because still unable to see changes??

Copy link
Member

@snitin315 snitin315 Jan 28, 2020

Choose a reason for hiding this comment

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

Share screenshots/GIF for current behaviour .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

It's still the same even after your commit?

Copy link
Member

@snitin315 snitin315 Jan 28, 2020

Choose a reason for hiding this comment

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

The validation is working fine with your changes.

Screenshot from 2020-01-28 20-40-26

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I am unable to see the changes in my local machine

Copy link
Member

Choose a reason for hiding this comment

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

@snitin315
Copy link
Member

snitin315 commented Jan 28, 2020

I am unable to get the changes as I earlier mentioned. Please confirm this

Earlier? where?

@mohitm15
Copy link
Contributor Author

I am unable to get the changes as I earlier mentioned. Please confirm this

Earlier? where?

Asked over Giiter

@snitin315
Copy link
Member

snitin315 commented Jan 28, 2020

Asked over Giiter

can you ask your query here again?

@snitin315
Copy link
Member

To see your changes you can click on reset passwod and then you have to change the link sent to your email.
change https://open-event-fe.netlify.com/ to https://deploy-preview-3882--open-event-fe.netlify.com/ in Reset Password Link

<i class="key icon"></i>
{{input type='password' name='password' placeholder=(t 'password') value=password}}
<div class="ui small basic icon buttons">
<button type="button" class="ui button" {{action 'showPassword' 'New'}}>
Copy link
Member

Choose a reason for hiding this comment

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

The showPassword button is not working. Have you defined the showPassword action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had already committed it locally? The indentation and toogle icon action both

<i class="key icon"></i>
{{input type='password' name='password' placeholder=(t 'password') value=password}}
<div class="ui small basic icon buttons">
<button type="button" class="ui button" {{action 'showPassword' 'New'}}>
Copy link
Member

Choose a reason for hiding this comment

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

This won't work, How is the form supposed to get the showPassword Action ?

Copy link
Contributor Author

@mohitm15 mohitm15 Jan 29, 2020

Choose a reason for hiding this comment

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

I am committed the action and indentation error . Just pushing it after previewing it on my local machine.

{{input type='password' name='password' placeholder=(t 'password') value=password}}
<div class="ui small basic icon buttons">
<button type="button" class="ui button" {{action 'showPassword' 'New'}}>
<i class="{{if showPassNew 'hide' 'unhide'}} basic icon"></i>
Copy link
Member

Choose a reason for hiding this comment

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

This won't work, How is the form supposed to get the showPassNew Property ?

},
{
type : 'minLength[8]',
prompt : this.l10n.t('Your password must have at least {ruleValue} characters')
Copy link
Member

Choose a reason for hiding this comment

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

Add a field for confirmation of password .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@mohitm15
Copy link
Contributor Author

mohitm15 commented Jan 30, 2020

To see your changes you can click on reset passwod and then you have to change the link sent to your email.
change https://open-event-fe.netlify.com/ to https://deploy-preview-3882--open-event-fe.netlify.com/ in Reset Password Link

@snitin315
Do I need to push the commit every time if I make changes in steps to see whether its working.
As the link has # 3882 number in it.

@kushthedude
Copy link
Member

To see your changes you can click on reset passwod and then you have to change the link sent to your email.
change https://open-event-fe.netlify.com/ to https://deploy-preview-3882--open-event-fe.netlify.com/ in Reset Password Link

@snitin315
Do I need to push the commit every time if I make changes in steps to see whether its working.
As the link has # 3882 number in it.

To see if it's working you can use ember serve in your local machine !

@mohitm15
Copy link
Contributor Author

To see your changes you can click on reset passwod and then you have to change the link sent to your email.
change https://open-event-fe.netlify.com/ to https://deploy-preview-3882--open-event-fe.netlify.com/ in Reset Password Link

@snitin315
Do I need to push the commit every time if I make changes in steps to see whether its working.
As the link has # 3882 number in it.

To see if it's working you can use ember serve in your local machine !

Does not show any changes!

@kushthedude
Copy link
Member

kushthedude commented Jan 30, 2020 via email

@mohitm15
Copy link
Contributor Author

Then it's not working

On Fri, 31 Jan, 2020, 00:02 Mohit Maroliya, @.***> wrote: To see your changes you can click on reset passwod and then you have to change the link sent to your email. change https://open-event-fe.netlify.com/ to https://deploy-preview-3882--open-event-fe.netlify.com/ in Reset Password Link @snitin315 https://github.com/snitin315 Do I need to push the commit every time if I make changes in steps to see whether its working. As the link has # 3882 number in it. To see if it's working you can use ember serve in your local machine ! Does not show any changes! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3882?email_source=notifications&email_token=AKQMTLU3LINX5ZLVQP5KM43RAMMMFA5CNFSM4KMSMBZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKMBDPY#issuecomment-580391359>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLQV6ZAGALLBEOZQ2O3RAMMMFANCNFSM4KMSMBZQ .

Then it's not working

On Fri, 31 Jan, 2020, 00:02 Mohit Maroliya, @.***> wrote: To see your changes you can click on reset passwod and then you have to change the link sent to your email. change https://open-event-fe.netlify.com/ to https://deploy-preview-3882--open-event-fe.netlify.com/ in Reset Password Link @snitin315 https://github.com/snitin315 Do I need to push the commit every time if I make changes in steps to see whether its working. As the link has # 3882 number in it. To see if it's working you can use ember serve in your local machine ! Does not show any changes! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3882?email_source=notifications&email_token=AKQMTLU3LINX5ZLVQP5KM43RAMMMFA5CNFSM4KMSMBZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKMBDPY#issuecomment-580391359>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLQV6ZAGALLBEOZQ2O3RAMMMFANCNFSM4KMSMBZQ .

Its not working even for the minor changes I made

@snitin315
Copy link
Member

snitin315 commented Jan 31, 2020

@mohit15 change https://open-event-fe.netlify.com to http://localhost:4200/ in reset password link .

Make sure ember serve is running.

@snitin315
Copy link
Member

@mohitm15 updates??

@mohitm15
Copy link
Contributor Author

@kushthedude Check the final status http://localhost:4200/reset-password?token=324700420269303046106645931387057135140 ,
I will squash commit as soon as travis build passes.

{{/if}}
<div class="ui small basic icon buttons">
<button type="button" class="ui button" {{action 'showNewPassword'}}>
{{#if showNewPass}}
Copy link
Member

Choose a reason for hiding this comment

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

A lot of duplication in here. Please reduce it and make the code optimal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then there will be no space in between both the textfields, thats why repeated the else part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ezgif com-video-to-gif (1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is implemented similar to that in sign up part

Copy link
Member

Choose a reason for hiding this comment

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

Goal is to improve the code as we move on. "It was implemented this way" doesn't really work

Remove duplication as much as possible

{{/if}}
<div class="ui small basic icon buttons">
<button type="button" class="ui button" {{action 'showNewPassword'}}>
{{#if showNewPass}}
Copy link
Member

Choose a reason for hiding this comment

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

Goal is to improve the code as we move on. "It was implemented this way" doesn't really work

Remove duplication as much as possible

@kushthedude
Copy link
Member

Stale!

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.

Missing password validation in forgot password window

3 participants