feat(Password): Allow special characters in password #1402
Conversation
Add more interactive password requirements indicator. Add password strength indicator. #1049
There was a problem hiding this comment.
Feature works great. I have some comments/questions:
- Do we want to require symbols? This would make passwords more difficult to remember, especially students.
- Always showing password strength indicator by default when the pages load feels a bit off because I haven't typed in anything yet. Should it show up once you start typing a password?
- And also maybe @breity can style the indicator a bit to fit into the page better and ensure contrast (a11y)?
- Change success message from "Password Changed" to "Successfully changed password" to make it more friendly?
- Code improvements
- See inline comments
- See if you can remove all duplications?
- For the new tests in this change set
- change it('should disable ...') to it('disables ...') https://github.com/CareMessagePlatform/jasmine-styleguide#speak-human?
- try to change test blocks to follow Arrange-Act-Assert pattern (https://github.com/CareMessagePlatform/jasmine-styleguide#arrange-act-assert) ?
...pp/forgot/student/forgot-student-password-change/forgot-student-password-change.component.ts
Outdated
Show resolved
Hide resolved
...pp/forgot/teacher/forgot-teacher-password-change/forgot-teacher-password-change.component.ts
Outdated
Show resolved
Hide resolved
| function generateObservableResponse(arg: string | any, isSuccess: boolean): Observable<any> { | ||
| return isSuccess ? generateSuccessObservable(arg) : generateErrorObservable(arg); | ||
| } | ||
|
|
||
| function generateSuccessObservable(arg: string | any): Observable<any> { | ||
| return new Observable((observer) => { | ||
| observer.next(generateSuccessResponseValue(arg)); | ||
| observer.complete(); | ||
| }); | ||
| } | ||
|
|
||
| function generateSuccessResponseValue(arg: string | any): any { | ||
| return typeof arg === 'string' ? { messageCode: arg } : arg; | ||
| } | ||
|
|
||
| function generateErrorObservable(arg: string | any): Observable<any> { | ||
| return new Observable((observer) => { | ||
| observer.error(generateErrorResponseValue(arg)); | ||
| observer.complete(); | ||
| }); | ||
| } | ||
|
|
||
| function generateErrorResponseValue(arg: string | any): any { | ||
| return typeof arg === 'string' ? { error: { messageCode: arg } } : { error: arg }; |
There was a problem hiding this comment.
Reduce duplication? Do you need generateSuccessObservable() and generateErrorObservable(), or can we do it with a ternary in generateObservableResponse()?
function generateObservableResponse(arg: string | any, isSuccess: boolean): Observable<any> {
return new Observable((observer) => {
observer.next(isSuccess ? generateSuccessResponseValue(arg) : generateErrorResponseValue(arg));
observer.complete();
});
}
src/app/password/new-password-and-confirm/new-password-and-confirm.component.spec.ts
Outdated
Show resolved
Hide resolved
src/app/password/new-password-and-confirm/new-password-and-confirm.harness.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
New changes look good.
We still need to discuss/address these:
- Always showing password strength indicator by default when the pages load feels a bit off because I haven't typed in anything yet. Should it show up once you start typing a password?
- Maybe @breity can style the indicator a bit to fit with the rest of the page better and ensure that a11y standards are met (e.g. has good contrast)?
|
@breity I needed to add this to new-password-and-confirm.component.scss in order to set the width of the menu since the default width caused some text to wrap. Is there a better way to do this without ng-deep? |
- Ensure sufficient contrast on light backgrounds
|
I removed the ng-deep requirement by setting view encapsulation to none for the component. Just need to be careful to make sure component styles don't modify any styles used elsewhere in the app when doing so. I also cleaned up the styles/layout a bit and addressed text color contrast for a11y. I removed the changing color for the text indicating password strength ("Very Weak", "Weak", ...) because it's difficult to find colors with sufficient contrast in the yellow and orange ranges unless they're rather dark. I think the colors on the strength meter combined with text itself works fine. |
geoffreykwan
deleted the
issue-1049-allow-special-characters-in-password
branch
Changes
Test
Make sure setting a password in these locations still works and validates with the new requirements
Closes #1049