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

css: Add nightmode variant for fetching more messages loading indicator #12527

Merged
merged 1 commit into from Jun 7, 2019

Conversation

alexandraciobica
Copy link
Collaborator

Fixes: #12522

Added white color to the loading spinner that is displayed when more messages are fetched.

The first GIF I just changed the code so that the spinner is displayed all the time so the change can be seen easier.
spinner2
For the second one, I am not sure why the spinner is not displayed all the times.
spinner

@zulipbot
Copy link
Member

zulipbot commented Jun 7, 2019

Hello @zulip/server-message-view members, this pull request was labeled with the "area: message view" label, so you may want to check it out!

@timabbott timabbott merged commit 161d196 into zulip:master Jun 7, 2019
@timabbott
Copy link
Sponsor Member

Awesome, thanks for fixing!

For the second one, I think what's going on is this:

  • The actual fetch from the server is extremely fast because it's the dev environment with no latency, so it only spins for a fraction of 100ms before having the data. It then freezes for a moment while the browser does a rerender to add the newly fetched messages.

This seems not a huge problem but not ideal; users who are self-hosting and have a fast network will likely see this funny behavior as well. From reading the message_fetch.js code, it looks like we're already calling the message_scroll.hide_loading_older function after doing the request to insert messages into the DOM (it goes through the cont argument). So I think our options are to add some sort of delay or animation in the hide function, or maybe to try to trigger the browser to finish its repaint earlier? I think it's worth spending 15 minutes playing around with things like adding a setTimeout(, 0) in exports.hide_loading_older to see if that helps, but it might not be a super solvable problem.

Also, do we need to use rgb in this context, or can we use HSL? We generally aim for HSL across the codebase. I notice via git grep 'rgb(' that we have a few things that are fill or similar rules that use RGB; assuming those all support HSL, we should probably convert them all and update our linter rules to ban rgb(; @amanagr has been in the linter codebase and can probably help with that.

@amanagr
Copy link
Member

amanagr commented Jun 7, 2019

sure! Added to my todo list.

@alexandraciobica alexandraciobica deleted the night-loader branch June 10, 2019 08:19
@alexandraciobica
Copy link
Collaborator Author

@timabbott I tried playing with setTimeout but there is no difference. But I observed that the funny behavior happens only when you scroll very fast, which usually doesn't happen. If you scroll with the speed like searching for a message or reading, then it looks ok.

We don't need to use the rgb here, I used it because there was another rgb above to the place where I added the color. I can add changing the rgbs to hsl on my todo list.

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.

Loading indicator for fetching more messages does not have a night theme variant
4 participants