-
Notifications
You must be signed in to change notification settings - Fork 5
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
Extract password edit from profile #644
base: master
Are you sure you want to change the base?
Conversation
This one can be fixed in this PR, too. |
@@ -0,0 +1,127 @@ | |||
<link rel="import" href="../../bower_components/polymer/polymer.html"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe its better if we use ES6 class-based syntax for this new element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now, right?
|
||
<dom-module id="my-area-password"> | ||
<template> | ||
<style include="iron-flex iron-flex-alignment"></style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use flexbox directly to style the content?
8c8db69
to
795f0e4
Compare
Just tested it (both with a correct and incorrect password), seems to be broken. Doesn't give any feedback about changes and when inspecting the network the response is a 403 (Access Denied). |
Yeah, the 403's were because it did work the first time, but upon the second time trying, the password was already changed. The things that are broken are the events. |
Could someone test this? I’m pretty sure that everything is fixed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works when correct password is filled in and new password typed correctly twice.
When its not supposed to work because the password is not correct or the new password does not meet the password requirements (although that is currently not running, it's already in master on the API side), it is not clear to the user why because it just states "Password update failed".
I don't think we should merge until some sort of explanation of the password requirements is in here and the reason of update failure is made clear to the user. I can imagine that that would require changes to the API.
@timovanasten and @martijnjanssen can you have a look at this pull request? |
@ArdyZ Boy, that is a while ago haha. I believe the code in this PR adding the functionality to change passwords was functioning (at the time at least), but I requested some changes from a user experience point of view. The API enforces some constraints on the password, but these are not communicated to the user. A simple piece of text with the password requirements should suffice. For example, something like "Password should be x characters long and ....". I don't have the environment set up to work on this project atm, so if you could verify that this still works and add the text with the password requirements to the element then this could be merged if you ask me. |
This is a first quick change, I still have some ideas for my-area, so the exact styling is not that important here I think.