renderer: Improve loading spinner.

[nooblag]

---

What's this PR do?
Improve loading GIF.

Any background context you want to provide?
Current GIF is archaic.

Screenshot

You have tested this PR on:

• Windows
• Linux/Ubuntu
• macOS

[andersk]

Please don’t open and close multiple separate pull requests for the same topic. You can always update an existing pull request by pushing to the same branch, using a force push if necessary.

The comment in the SVG points to loading.io, which has 4 available licenses. Which license is this one under?

[nooblag]

License for the spinner is Free

[chrisbobbe]

Thanks, @nooblag!

I spend most of my time in the zulip/zulip-mobile repo, but I saw this and your message in CZO and thought I might make a few comments that I hope will help move it along. 🙂

The answer to Anders's question about the license should appear in the code comment itself. It's a natural thing for someone to be curious about when they're looking at the code, not just when they're looking at this PR discussion in the GitHub UI. Most people reading the code won't go to GitHub and see this PR.

I see what look like two different changes in the one commit here:

1. A GIF file is changed.
2. An SVG file is added, and something starts pointing to it instead of the GIF file from (1).

I wonder if this should be two commits instead of one. Do the changes make more sense when seen together, or separately? See the guide to clear and coherent commits.

Does (2) remove the last reference to the GIF file? If so, why is the GIF file changed instead of removed? If not, where is the GIF file still used? Let's make the answers clear in the commit(s).

Since I work mostly on mobile, I've come to view the commit without the context of any previous discussion of its changes. Commits should be written to an audience that includes people who weren't part of any particular discussion. 🙂 It would be helpful to have a link to any previous discussion, in the commit message—but also I think it should be possible to summarize the reasons for the changes without relying on any previous discussion at all. A few words about why we think these particular changes improve the loading spinner would be great.

I see you've posted a screenshot above; great! If it were me, I'd mention that in the commit message, with a link, to make it easier for people to find. Including a link assumes GitHub will still exist and the link will still be valid at the time it's followed—but these assumptions should hold for a good long time.

[nooblag]

Heyas, I'm hoping now the Electron upgrade has been done, this PR can be merged to update the archaic spinner that is still hanging around ;)

[nooblag]

Hi there, any chance this will ever get merged?

[chrisbobbe]

It looks like it's still waiting for a response to my review, above. Also, the GitHub UI is reporting some failing CI checks, and the branch contains a merge commit (Zulip doesn't use those).

[nooblag]

Hi there, thanks for getting back.

I have a few responses then.

The first regarding multiple commits. For background, in the past when I've donated my time and energy to making small improvements like this for Zulip, I've done so with multiple commits, but have been told that there are too many commits and that I should squash them into one (which is what I tried to do here). So not sure what to do here regarding conflicting information on that front. Likewise with the merge commit. I've followed the Zulip documentation on development (which also by the way mentions that merge commits aren't a bad thing) so I'm not sure what is desired here, pending further instructions on what I should be asking git to do in regards to each person's various preferences.

In regards to the license, am I essentially being asked to modify the spinner loading.io code to show its license? That could be done, and likewise with removing the GIF file entirely as a separate commit, if people accept the SVG instead, and that's what will get us to this PR being merged.

[chrisbobbe]

Our practice in the Zulip project is that “each commit is a minimal coherent idea”; details in this doc. Sometimes that requires splitting one commit into multiple, sometimes squashing multiple commits into one.

Here's a suggestion: You could amend the commit to delete the GIF, with the idea being simply "replace an old GIF with a new SVG". Did you want to use the new version of the GIF for anything? (If so, where did it come from, when should it be used instead of the SVG, etc.?)

Please do remove the merge commit, following what the doc says just after "merge commits aren’t bad":

however, Zulip doesn’t use them. Instead Zulip uses a forked-repo, rebase-oriented workflow.

Please feel free to post in #git help in the Zulip Development Community with any questions about how to use Git or how to apply our commit discipline.

For the license, I suggest writing your answer to Anders's question in a brief code comment in app/renderer/css/main.css, just above the line responsible for using the SVG file in the app. Since the comment starts being useful exactly when the app starts using the SVG, it makes sense to add that comment in the same commit.

[nooblag]

Hi @chrisbobbe Thanks for that. It's very helpful. How are these changes looking then?

[chrisbobbe]

Thanks for the revision! The history looks better now; the next step will be a review from @andersk.

[nooblag]

Okay great, thanks :)

[nooblag]

Just responding to the bot message. I can't see a conflict. Someone with write access want to have a look?

[nooblag]

A friendly ping on this.

[nooblag]

Dude, just say if you're never going to merge this, and I'll withdraw it. It shouldn't take years and years to accept PR for a spinner?