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

issue-98: Enable optimistic update for tags #99

Merged
merged 4 commits into from
Apr 17, 2019

Conversation

brandon-bogan
Copy link
Contributor

Search.js's updateRealTimeField function and Searcher's updateTags function now take onCompletion and onError callbacks, which get called once the update has been sent in but before the commit finishes.

SearchResultTags creates the callbacks, and knows to show the update to the user and stop loading/spinning once one of these is called.

Copy link
Collaborator

@luigivaldez luigivaldez left a comment

Choose a reason for hiding this comment

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

Hey Brandon, can you make these changes on your tag_autocomplete branch instead. This way, you've created a second pull request instead of just augmenting the first one and it's not really possible to reconcile them both together... (If you commit changes to a branch that's already the source of a pull request, the changes are added to the PR automatically—GitHub can even let each of us see the changes since the previous review...)
Thanks!!

@brandon-bogan
Copy link
Contributor Author

Hey Brandon, can you make these changes on your tag_autocomplete branch instead. This way, you've created a second pull request instead of just augmenting the first one and it's not really possible to reconcile them both together... (If you commit changes to a branch that's already the source of a pull request, the changes are added to the PR automatically—GitHub can even let each of us see the changes since the previous review...)
Thanks!!

Done! See brandon-bogan@8a4810b for the merge into the tag_autocomplete branch

@@ -719,8 +719,8 @@ class Searcher extends React.Component<SearcherDefaultProps, SearcherProps, Sear
/**
* Update the list of tags for the given document.
*/
updateTags(tags: Array<string>, docId: string): Promise<any> {
return this.search.updateRealtimeField(docId, FieldNames.TAGS, tags);
updateTags(tags: Array<string>, docId: string, onCompletion: () => void, onError: (error: string) => void): Promise<any> {
Copy link
Contributor

@klk1010 klk1010 Apr 16, 2019

Choose a reason for hiding this comment

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

I might be missing something, but I don't see a place where updateTags is used. This change isn't harmful, but I'd expect to see any place using updateTags to be updated as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

nm, I see it being used through context and you already updated that.

@klk1010
Copy link
Contributor

klk1010 commented Apr 17, 2019

If this is being included in PR#97, can this PR be closed?

@luigivaldez luigivaldez merged commit 5e2e2f6 into attivio:master Apr 17, 2019
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.

4 participants