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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Swapnilr1
Copy link
Collaborator

@Swapnilr1 Swapnilr1 commented Feb 13, 2019


What's this PR do?
Fixes issue #533

Any background context you want to provide?

Screenshots?

You have tested this PR on:

  • Windows
  • 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
Copy link
Collaborator Author

@akashnimare Please review.

@Swapnilr1
Copy link
Collaborator Author

@akashnimare ping.

@Swapnilr1
Copy link
Collaborator Author

@akashnimare Please review this.

@akashnimare akashnimare requested a review from abhigyank March 14, 2019 08:03
@akashnimare
Copy link
Member

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.

@rishig
Copy link
Member

rishig commented Mar 19, 2019

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

@abhigyank
Copy link
Collaborator

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.

@rishig
Copy link
Member

rishig commented Mar 19, 2019

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.

@zulipbot
Copy link
Member

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.

Base automatically changed from master to main January 22, 2021 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants