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

impove avatar loading #18768

Merged
merged 24 commits into from Aug 6, 2019

Conversation

@mmaxim
Copy link
Member

commented Aug 4, 2019

Patch does the following:

  1. Refactor attachment HTTP server so we can use the server in more packages in the service. Create manager.Srv in kbhttp with the factored out code.
  2. Adds a new generic token in front of manager.Srv.
  3. Add protocol for keeping JS current with the address and token of HTTP server.
  4. Add avatars.Srv for serving avatars via the HTTP server.
  5. Refactor avatars JS to strip out all old code that invoked RPCs to get URLs, and replace with simpler code that just gets the URLs a priori.

mmaxim added some commits Aug 4, 2019

wip
ts
wip
fix

@mmaxim mmaxim requested a review from chrisnojima Aug 5, 2019

@mmaxim mmaxim marked this pull request as ready for review Aug 5, 2019

@mmaxim mmaxim requested a review from joshblum Aug 5, 2019

fix
@@ -102,6 +110,10 @@ function* loadDaemonBootstrapStatus(
yield Saga.put(loadedAction)
// request follower info in the background
yield RPCTypes.configRequestFollowerInfoRpcPromise({uid: s.uid})
// set HTTP srv info
yield Saga.put(
ConfigGen.createUpdateHTTPSrvInfo({address: s.httpSrvInfo.address, token: s.httpSrvInfo.token})

This comment has been minimized.

Copy link
@chrisnojima

chrisnojima Aug 5, 2019

Contributor

You don't need to make this. Just handle it in the reducer

This comment has been minimized.

Copy link
@mmaxim

mmaxim Aug 5, 2019

Author Member

I did it like this because this action also gets fired as a result of a notification from the service. With this, there is just one place in the reducer where these values are updated, instead of the bootstrap handler doing it, and then a handler for the notification. Seemed cleaner this way.

shared/actions/teams.tsx Outdated Show resolved Hide resolved
@mmaxim

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

cc @songgao

mmaxim added some commits Aug 6, 2019

@mmaxim mmaxim merged commit 70aea10 into master Aug 6, 2019

1 of 2 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
ci/circleci Your tests passed on CircleCI!
Details

@mmaxim mmaxim deleted the mike/HOTPOT-352 branch Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.