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.

Sign up for GitHub

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

Jump to bottom

Loading indicator #1485

Merged
merged 13 commits into from
Aug 3, 2016
Merged

Loading indicator #1485

merged 13 commits into from
Aug 3, 2016

Conversation

foot
Copy link
Contributor

@foot foot commented May 12, 2016 •
edited
Loading

Fixes #1176

@foot foot force-pushed the loading-indicator branch from 12a7aba to ad2eb32 Compare May 12, 2016 11:35
@foot foot changed the title [WIP] Loading indicator Loading indicator May 12, 2016
@foot
Copy link
Contributor Author

foot commented May 12, 2016

Alright! So it didn't end up being very exciting. 💍 . For now.

Do you see it for very long on your machine?

davkal
davkal reviewed May 12, 2016
View reviewed changes
client/app/scripts/reducers/root.js
@@ -70,7 +71,7 @@ export const initialState = makeMap({
updatePausedAt: null, // Date
version: '...',
versionUpdate: null,
websocketClosed: true,
websocketClosed: false,

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@davkal
Copy link
Contributor

davkal commented May 12, 2016

I like the entertainment, but only when I have to wait. On fast loads, it looks a bit distracting. How about firing off a timer that reevaluates the state after x seconds, and only then fires an action to display the indicator.

@foot
Copy link
Contributor Author

foot commented May 12, 2016

Yep, good call

On 12 May 2016 at 17:01, David [email protected] wrote:

I like the entertainment, but only when I have to wait. On fast loads, it
looks a bit distracting. How about firing off a timer that reevaluates the
state after x seconds, and only then fires an action to display the
indicator.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1485 (comment)

@foot
Copy link
Contributor Author

foot commented May 12, 2016

Could you do this purely in css? Perhaps... do the settimeout onMount to
add a class...

On 12 May 2016 at 17:02, Simon Howe [email protected] wrote:

Yep, good call

On 12 May 2016 at 17:01, David [email protected] wrote:

I like the entertainment, but only when I have to wait. On fast loads, it
looks a bit distracting. How about firing off a timer that reevaluates the
state after x seconds, and only then fires an action to display the
indicator.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1485 (comment)

@davkal
Copy link
Contributor

davkal commented May 12, 2016

CSS is worth a try, maybe transition-delay could help. Or you piggy-back on the animation starting out transparent and use animation delay.

@davkal davkal assigned foot and unassigned davkal May 13, 2016
@foot foot force-pushed the loading-indicator branch from ddc7b08 to 63aa05c Compare June 2, 2016 09:39
@rade
Copy link
Member

rade commented Jul 12, 2016

Is this meant to fix #1176? What's left to do here?

@foot
Copy link
Contributor Author

foot commented Jul 14, 2016

Yes, this should help w/ #1176

Left to do:

  • Delay the appearance of the indicator for a second or two as its a distracting if flashing up for ~200ms.

@davkal davkal force-pushed the loading-indicator branch from 63aa05c to 20f70e7 Compare July 25, 2016 08:52
foot added 12 commits July 28, 2016 16:28
@foot
Adds basic "loading topologies" indicator for initial load.
9a0feb3 
Which can take a while sometimes.
@foot
Adds a loading indicator for the initial node-load
7d14a78 
- Sometimes can take a second to get the initial nodes
- Not doing the transition between topos atm as its a bit distracting
  popping up and down.
@foot
No title case on the loading messages
7dfae47
@foot
Avoid blurry text in topo loading message
5df5e40
@foot
Handle loading message for empty topos correctly.
04f4071 
It was showing infinite loading...
@foot
Smooth fade-out of node-loading message
335cc11
@foot
Restore log error message
b92a990
@foot
More personal loading messages
74e0540
@foot
Less apologetic ui messages
9a77b0d
@foot
New and improved loading messages.
50a1c11 
Are these getting better or worse...
@foot
Typo and removing some more mutatey sounding verbs
fba40de
@foot
Pulls loading out into new cmp, adds delayed show for loading
6e3bec8
@foot foot force-pushed the loading-indicator branch from 20f70e7 to 6e3bec8 Compare July 28, 2016 16:00
@foot
Copy link
Contributor Author

foot commented Jul 28, 2016

Ready for re-review!

  • Pulled out loading into new cmp
  • DelayedShow cmp wrapper.

@foot foot removed their assignment Aug 1, 2016
davkal
davkal reviewed Aug 1, 2016
View reviewed changes
client/app/scripts/utils/delayed-show.js
this.showTimeout = setTimeout(() => this.setState({ show: true }), this.props.delay);
}

cancelShow() {

This comment was marked as abuse.

Sign in to view

@davkal
Copy link
Contributor

davkal commented Aug 1, 2016

Looks great. There is still the theoretical state when the UI loads w/ no backend present, then you get the load indicator (you should get an error instead). It will become less theoretical when the UI code is served by a webserver as opposed to the scope app itself.
Other than that, LGTM

@rade
Copy link
Member

rade commented Aug 1, 2016

Yes, this should help w/ #1176

"help with" or "fix"?

@foot
Copy link
Contributor Author

foot commented Aug 1, 2016

This will show a loading screen until the first delta is received which I believe is what is happening in #1176. This PR will fix that. Updated the description.

@davkal davkal mentioned this pull request Aug 2, 2016
Closed
@foot foot merged commit e7a0a96 into master Aug 3, 2016
@foot foot deleted the loading-indicator branch August 3, 2016 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@davkal davkal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@foot @davkal @rade