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

Pages: check for recent errors, add placeholder for errored requests. #1247

Merged
merged 1 commit into from
Dec 7, 2015

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Dec 3, 2015

Fixes #358, where after losing internet connection, the pages placeholder would glitch between a loading placeholder and an empty placeholder. This PR adds a method hasRecentError to the store, so the we can limit the number of actions dispatched from the view when the store enters that state. This also adds a simple errored placeholder to pages.

Testing instructions:

  1. In a fresh browser tab, open: http://calypso.localhost:3000/posts
  2. Select a site if prompted
  3. Turn off your wifi
  4. Click on the Pages sidebar link

Before:
glitch
After:
after

cc @rralian @rickybanister

@gwwar gwwar added Pages [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Dec 3, 2015
@gwwar gwwar self-assigned this Dec 3, 2015
@rickybanister
Copy link

I have a question about this approach: we have notices (dev-only at the moment) that display an offline message. We chose not to put those in production (I'm not remembering the details), but I think it's a good idea to talk through a unified approach to all sections while we're addressing this issue.

The disconnected notice flashes as you lose and regain connection, whereas this issue occurs when attempting to switch sections. I think the proposed solution is better than seeing a placeholder flash forever and certainly better than seeing a false 'you don't have any pages' message, I just want to make sure this is something we could do for every section.

@gwwar
Copy link
Contributor Author

gwwar commented Dec 4, 2015

I just want to make sure this is something we could do for every section.

Certainly! Which is why I asked for a design review. :) I would probably suggest opening up new issues/PRs for the other affected areas so this PR won't grow too large. At the very least I'll create a few new issues around making sure the experience between Posts/Pages are similar.

@rickybanister
Copy link

I think the design works! Much better than the placeholder and much better than an inaccurate and scary empty content message.

@gwwar gwwar force-pushed the fix/pages-placheolder-disconnected-358 branch from 86736bd to dc1685b Compare December 4, 2015 19:19
@gwwar gwwar added this to the Core: Iteration 17 milestone Dec 4, 2015
@rralian
Copy link
Contributor

rralian commented Dec 7, 2015

Excellent... we limit the requests to a reasonable throttle. It comes back if you fix your connection. Maybe in future we could get fancier with our connection detection, and queue up failed attempts (across different endpoints) that we know we want to re-try once we detect the connection has been re-established, versus waiting for the 30-second throttle to iterate. But I think this is the right approach to fix this issue now. :shipit:

@rralian rralian added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 7, 2015
@gwwar
Copy link
Contributor Author

gwwar commented Dec 7, 2015

👍 Adding in a circuit breaker pattern could be a great enhancement in the future, maybe using: https://github.com/yammer/circuit-breaker-js

@gwwar gwwar removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Dec 7, 2015
gwwar added a commit that referenced this pull request Dec 7, 2015
…nected-358

Pages: check for recent errors, add placeholder for errored requests.
@gwwar gwwar merged commit a031db9 into master Dec 7, 2015
@gwwar gwwar deleted the fix/pages-placheolder-disconnected-358 branch December 7, 2015 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants