Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

[WIP] User refactor #752

Closed
wants to merge 3 commits into from
Closed

[WIP] User refactor #752

wants to merge 3 commits into from

Conversation

rhutchison
Copy link
Contributor

Up for discussion.

This is a work in progress. There is incomplete code and more to be done. I am looking for general direction feedback, not a code-review.

support multiple e-mails

client-side validation

update signup form

start or emails list.  need to implement add and edit modal, and ability to switch primary.

showErrors enhancements.

formatting.
@mleanos
Copy link
Member

mleanos commented Aug 4, 2015

@rhutchison You might consider breaking these changes up into multiple PR's. You have quite a bit going on here; with UI enhancements & back-end refactoring. It's getting a little unmanageable. It's going to hinder the ability for other developers to collaborate with you on this. I already experienced this for myself with this branch.

You may gain some clarity, if you can get back to nuetral and give yourself a little more focus. Furthermore, these changes might get out quicker if they have a little more autonomy. I hope this helps; as I know you're deep in it right now.

mongoose = require('mongoose'),
config = require('../config/config'),
mg = require('../config/lib/mongoose');
mongoose = require('mongoose'),
Copy link
Member

Choose a reason for hiding this comment

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

These formatting changes should be in a separate PR. If it were strictly updating some code, then 2 commits, one for the functionality changes, and one for the formatting would be useful so the diff isn't too confusing. I barely noticed the .email to .primaryEmail change.

@lirantal
Copy link
Member

lirantal commented Aug 5, 2015

@rhutchison another comment on the PR - can you please describe in a sentence/paragraph what this PR does? since this isn't some straight-forward, very specific and small PR, it requires someone to seriously code review everything to figure out all the bits and bites of what this PR does.

Would be helpful if you can explain it in simple words, what it affects, and what significant changes it makes so we can understand purpose and risks in this PR.

Thanks :)

@ilanbiala
Copy link
Member

@rhutchison @mleanos @lirantal I think we're too far past this and this user implementation isn't better than what @sielay had, so I'm just going to close and we can possibly work towards something better for 0.5.0 or 0.6.0.

@ilanbiala ilanbiala closed this Oct 27, 2015
@sielay
Copy link

sielay commented Nov 10, 2015

@ilanbiala I have not time to sync with you, but copy what you want from CleanMean repo

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants