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

Show loading indicator on topology changes #2232

Merged
merged 2 commits into from
Feb 20, 2017
Merged

Conversation

jpellizzari
Copy link
Contributor

@jpellizzari jpellizzari commented Feb 10, 2017

Implements #1804

When a user changes topologies via click on the topology filter, or by clicking 'SHOW IN ', the loading indicator will show after 1 second. It will be cleared by the first nodes delta data arriving via websockets.

@jpellizzari
Copy link
Contributor Author

@simon Could you take a look por favor?

@davkal davkal requested review from fbarl and removed request for foot February 13, 2017 15:39
@davkal davkal unassigned foot Feb 13, 2017
@jpellizzari jpellizzari force-pushed the 1804-loading-topologies branch from aa550a8 to 404f0fc Compare February 13, 2017 17:54
Copy link
Contributor

@fbarl fbarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments about the UI:

  • When you are in the graph view mode and you click on the topology that's already selected, the loader shows up on top of the graph that is already there. One solution here would be to simply move your added line(s) into the if-clause above, so that nodesLoaded is set to false only when the topology actually changes. As a side note, I'd also extract all this shared code between CLICK_SHOW_TOPOLOGY_FOR_NODE and CLICK_TOPOLOGY into a switchTopology helper, just because there's a lot of repeated logic.
  • Switch to the table mode happens now every time I change back to a big topology (instead of just the first time), which I think can be slightly annoying. This is because of the condition in line 521 which I think we could be fixed easily if we also add a boolean nodesLoadedFirstTime or something like that to the global state... or perhaps you can find a more elegant solution.

@jpellizzari jpellizzari force-pushed the 1804-loading-topologies branch from b44e682 to 58fa1c9 Compare February 15, 2017 05:39
@jpellizzari
Copy link
Contributor Author

@fbarl Updated

Copy link
Contributor

@fbarl fbarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jpellizzari jpellizzari merged commit a07d01b into master Feb 20, 2017
@jpellizzari jpellizzari deleted the 1804-loading-topologies branch February 20, 2017 02:52
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.

3 participants