-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Contact list #6702
Contact list #6702
Conversation
@jennypollack Can you include a description of the PR and some screen shots or screen captures of it being used? |
@jennypollack here is some styling edits! |
http://g.recordit.co/Wu0ITQuLeK.gif @danjm gif of settings flow |
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.
Code looks good overall. Most changes I've suggested are related to code patterns or logic simplifications. These are none critical.
The only truly important changes I suggest relate to how we handle and show validation errors in the add-contact
and edit-contact
components
ui/app/pages/settings/contact-list-tab/add-contact/add-contact.component.js
Outdated
Show resolved
Hide resolved
ui/app/pages/settings/contact-list-tab/contact-list-tab.component.js
Outdated
Show resolved
Hide resolved
ui/app/pages/settings/contact-list-tab/view-contact/view-contact.component.js
Outdated
Show resolved
Hide resolved
ui/app/pages/settings/contact-list-tab/edit-contact/edit-contact.component.js
Outdated
Show resolved
Hide resolved
ui/app/pages/settings/contact-list-tab/edit-contact/edit-contact.component.js
Outdated
Show resolved
Hide resolved
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.
The approval I just made was supposed to be a "Request Changes" 😆
We should also get some design review in here @bdresser |
No description provided.