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

feat(auth) confirm password #471

Closed
wants to merge 2 commits into from
Closed

feat(auth) confirm password #471

wants to merge 2 commits into from

Conversation

thomporter
Copy link

This is something important to my clients, so I thought it would be good to add it to the generator.
Also adding the attribute to labels w/ associated IDs on inputs so clicking labels will
do as they should - focus the input.
Changes:
adding bower dep for angular-confirm-field (https://github.com/wongatech/angular-confirm-field)
adding angular dependency 'ng.confirmField' to app/index.js
adding confirm password fields to signup pages (html & jade)
adding attribute to labels with matching ids on their corresponding inputs
repeating previous 2 steps for settings pages (html & jade)

This is something important to my clients, so I thought it would be good to add it to the generator.
Also adding the  attribute to labels w/ associated IDs on inputs so clicking labels will
do as they should - focus the input.
Changes:
adding bower dep for angular-confirm-field (https://github.com/wongatech/angular-confirm-field)
adding angular dependency 'ng.confirmField' to app/index.js
adding confirm password fields to signup pages (html & jade)
adding  attribute to labels with matching ids on their corresponding inputs
repeating previous 2 steps for settings pages (html & jade)
@thomporter
Copy link
Author

Ack, I didn't run the tests locally, my bad. The error is boiling down to this:

Error: [$injector:nomod] Module 'ng.confirmField' is not available! You either misspelled the module name or forgot to load it. If registering a module ensure that you specify the dependencies as the second argument.

When I look into the test/temp/ folder I can see the generated app and the dependency for ng.confirmField is there in the app.js file. Is there a separate place I need to list it for the test?

@kingcody
Copy link
Member

@thomporter, adding "angular-confirm-field": "~0.1.2" to test/fixtures/bower.json should resolve the problem. Let me know if that works for you.

Sorry, that doesn't seem to fix the issue; I just had a chance to check myself.

@kingcody
Copy link
Member

Wow, how silly of me... Karma is using phantom.js to test; in app/templates/karma.conf.js you'll see a files array, add: client/bower_components/angular-confirm-field/app/package/js/angular-confirm-field.min.js

@thomporter
Copy link
Author

Bah, I shoulda known about that file! =) Fixed & ready for merge!

@thomporter
Copy link
Author

I just saw "PRs directly to canary would make my job easier." in an older PR. I can do that for ya...

@kingcody
Copy link
Member

@thomporter, I was wondering if you wouldn't mind squashing those two commits. I think it would make more sense especially since the change log is generated from commits, and being that the first commit introduces the bug that the second one fixes; I see no reason to have them as separate.

@kingcody
Copy link
Member

I mean no offense, just a suggestion really :)

@thomporter
Copy link
Author

@kingcody none taken, I totally agree. Question though, I went to the canary branch to start working on it there, and I'm seeing on the settings page there's a new feature (edit email) which isn't in master. I haven't tested it yet, not sure if it's fully functional... Should I just edit that page and add the confirm box? I actually haven't gotten to the signup page in canary yet.... 😄

@thomporter thomporter closed this Aug 23, 2014
@kingcody
Copy link
Member

Absolutely. I do believe that was implemented by @chester1000 in his recent PR to improve 3rd party auth options and allow for multiple strategies per user.

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.

2 participants