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
add people to current page #900
Conversation
nelsonpecora
commented
Aug 30, 2017
- allows adding other people to the current page
- allows you to search through directory of people, to find the best person
- this does not notify the person added, but does put the page in their "My Pages" list
panes/add-person-to-page.vue
Outdated
addPersonToPage(user) { | ||
this.$store.dispatch('startProgress', 'save'); | ||
return this.$store.dispatch('updatePageList', { user }).then(() => { | ||
this.$store.dispatch('finishProgress', 'save'); |
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.
I'd recommend adding finishProgress
and closing the pane and such to the catch
for the promise as well, unless the error handling is already happening elsewhere
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.
Good point, I need to catch that if it fails for some reason
import { postJSON } from '../lib/core-data/api'; | ||
import { searchRoute } from '../lib/utils/references'; | ||
|
||
function buildUserQuery(query) { |
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.
nit:
buildUserQuery isn't a UI thing—by pulling it out of the Vue file it would be easier to test
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.
Eh, the logic for it is super simple. There's not much to test 😅
code LGTM modulo 2 nits. Want me to run it and test in a browser? |