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

topics: Fix failure to load from offline state. #2326

Closed
wants to merge 1 commit into from

Conversation

codebu5ter
Copy link
Collaborator

This fix first checks for the online status in TopicListScreen, and if found to be online, renders TopicList, which houses all the Redux logic and the call to Selectors that were originally in TopicListScreen.

Fixes #2310

This fix first checks for the online status in TopicListScreen, and if
found to be online, renders TopicList, which houses all the Redux logic
and the call to Selectors that were originally in TopicListScreen.

Fixes zulip#2310
@pulkonet
Copy link
Collaborator

Not entirely sure, but I don't think this is the right way to do this. Because if we go this way we would have to change all the screen in the same manner which would be quite a hassle. My take on this would be to change the fetching logic in a manner that you try to fetch the data after the network state changes from offline to online. What do you think?

@codebu5ter
Copy link
Collaborator Author

While you are correct, but at the same time, the only other component that will need such refactoring is the MessageListContainer/ MessageListWeb, since that's what displays the messages (both private and stream messages). I don't think any other screen would require this. Besides, this isOnline logic has to be implemented one way or the other and I've just implemented it inside the component and moved around a couple of functions calls.

@pulkonet
Copy link
Collaborator

You are right about implementing it one way or other. But consider a case where you have weak signals and are constantly losing and gaining them, that would create a lot of re-renders for the component but if we change the logic for fetching the messages that won't occur. Also, I understand that this might not be a very frequently occurring case, but this will exist, so taking the other path (which will separate fetching and rendering) sounds like a better choice.

@zulipbot
Copy link
Member

Heads up @codebu5ter, 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.

@codebu5ter
Copy link
Collaborator Author

I suppose that is a valid concern. I can modify the logic to include something like a flag that turns true and prevents re-renders, but that would seem hack-y and would not look pretty.
So, I'll look at ways to re-implement the same logic on the API. 👍

@pulkonet
Copy link
Collaborator

Yes. Correcting the API logic would be better and also you could directly implement the same corrected logic to Message fetching API.

@codebu5ter
Copy link
Collaborator Author

Indeed. That's what I intend to do.

@gnprice gnprice closed this Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Topics list fail to load if opened from an offline state
4 participants