Page MenuHomePhabricator

Add visual error handling to dashboard and detail
ClosedPublic

Authored by fdans on Aug 23 2017, 2:46 PM.

Details

Summary

Ref T171487

Adds an overlay component that can be embedded into any component in Wikistats. Currently only being used with connectivity problems, once Uniques is merged it should also handle incompatible metrics/families

Test Plan

Open the dashboard or the detail page with a throttled connection (in developer tools, you can change that under the Network menu).
To test errors, change the url of the metric in the configuration to an invalid value. The error overlay should appear over the metric card or detail page.

Diff Detail

Repository
rWIKISTATS analytics-wikistats-new
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fdans created this revision.Aug 23 2017, 2:46 PM
fdans edited the summary of this revision. (Show Details)Aug 23 2017, 2:46 PM
fdans updated this revision to Diff 1997.Aug 24 2017, 12:36 PM
  • adds arrow icon dep for placeholder
  • adds possible errors to status overlay
  • metric widget uses predefined error messages
  • adds error code handler
fdans updated this revision to Diff 2005.Aug 29 2017, 10:44 PM
  • Merge branch 'develop' of ssh://git-ssh.wikimedia.org/source/wikistats into error-handling

The code looks good overall, I left 2 nit-picky comments only.
But I tried to generate an error as described in the test plan (by changing the metric URL),
and both the dashboard and the detail page do not show any error message or overlay.
Not sure if I tested it right, see screenshot:

src/components/StatusOverlay.vue
3

Is the camelCase needed here?
Otherwise, we should stick to kebab-case for css classes no?
Also, I think we can stick to the standard of double quotes and no spaces around = for HTML code.

src/components/dashboard/MetricPlaceholderWidget.vue
5

I guess the style attribute is necessary here, as opposed to have it together with the css.
If not, then we should factor it out into a class in the css no?

fdans marked an inline comment as done.Sep 15 2017, 2:53 PM
fdans added inline comments.
src/components/dashboard/MetricPlaceholderWidget.vue
5

Semantic overrides the styles if I pass the inline styles to a class, even when using !important.

mforns added inline comments.Sep 15 2017, 4:41 PM
src/components/dashboard/MetricPlaceholderWidget.vue
5

K!

This revision was automatically updated to reflect the committed changes.
Milimetric added inline comments.
src/components/StatusOverlay.vue
3

The CSS rules should follow Semantic UI's style.

  • multi-word classes should be broken up into multiple classes unless it's tied to javascript or impossible for some crazy reason (so iconClass is fine if you have to, but overlay-text should be two words, or just one word with a more selective selector)
  • use double quotes in html, not single quotes
src/components/dashboard/MetricPlaceholderWidget.vue
5

nah, you just have to be more specific than the style from semantic. So you might need to do .ui.medium.statistic div.label {} for example. But this is probably not semantic, it's our own style definition, which probably uses !important too (that's my bad, I went a little crazy with it in the prototype). Same for all the style="font-family..."

src/components/dashboard/MetricWidget.vue
104

probably "Something went wrong" is better.