zulip-desktop
zulip-desktop copied to clipboard
Changed saveServerIcon() to not use request() on defaultIconUrl
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.
@akashnimare Please review.
@akashnimare ping.
@akashnimare Please review this.
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.
I kind of think we should just have something that looks clearly like an error state, since this should be a pretty rare condition.
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.
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.
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.