zulip-desktop icon indicating copy to clipboard operation
zulip-desktop copied to clipboard

Changed saveServerIcon() to not use request() on defaultIconUrl

Open Swapnilr1 opened this issue 6 years ago • 8 comments


What's this PR do? Fixes issue #533

Any background context you want to provide?

Screenshots?

You have tested this PR on:

  • [ ] Windows
  • [x] Linux/Ubuntu
  • [ ] macOS

This is corrected pull request (earlier it was #627). I had made changes in master branch, so it was difficult to correct that PR. @akashnimare @abhigyank Please review this and apologies for the delay.

Swapnilr1 avatar Feb 13 '19 20:02 Swapnilr1

@akashnimare Please review.

Swapnilr1 avatar Feb 26 '19 15:02 Swapnilr1

@akashnimare ping.

Swapnilr1 avatar Mar 03 '19 12:03 Swapnilr1

@akashnimare Please review this.

Swapnilr1 avatar Mar 14 '19 07:03 Swapnilr1

I'm holding this PR for a couple of reasons - a) We are still not sure whether we should use the default Zulip icon or not (this is when the org haven't set the realm icon) b) There are some discussions on CZO which suggest we should be using the default realm character icon We first need to make the decision on what exactly we want here and then go for the solution. cc @rishig, @abhigyank, @priyank-p.

akashnimare avatar Mar 19 '19 18:03 akashnimare

I kind of think we should just have something that looks clearly like an error state, since this should be a pretty rare condition.

rishig avatar Mar 19 '19 19:03 rishig

Before the realm character icon implementation, we had this Zulip icon as a fallback. Since moving to realm character icon implementation this use case has become almost minimal and should be a very rare occasion that the Zulip fallback icon is being used. So we can entire remove that part or keep it as is and merge this PR to avoid the occurrence of a harmless error.

abhigyank avatar Mar 19 '19 19:03 abhigyank

oh, I would say to actually remove the realm character icon fallback as well, but we can do that after we fix the underlying bug that is causing it to appear at all.

rishig avatar Mar 19 '19 20:03 rishig

Heads up @Swapnilr1, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

zulipbot avatar Aug 17 '19 14:08 zulipbot