Prevent flicker on Getting Started page #11826

ycombinator · 2017-05-16T19:26:28Z

What's in the box

This PR:

Prevents flicker of the chrome by starting out the chrome as invisible. Then, depending on the scenario being handled in the add_setup_work.js module's gettingStartedGateCheck function, makes the chrome visible again.
Consolidates the code that toggles the chrome visibility into a single module, add_setup_work.js, making it easier to reason about chrome visibility under various scenarios.
Extracts the three scenarios handled by the gettingStartedGateCheck function into their own functions for better readability.

Before this PR

After this PR

spalger

Can you please add some tests for gettingStartedGateCheck()? Happy to help bootstrap them if you're not sure where to start.

ycombinator · 2017-05-17T12:25:17Z

@spalger I created #11850 for functional tests that will exercise the paths through gettingStartedGateCheck(). I made it a separate PR hoping it's a bit easier to review.

epixa · 2017-05-17T12:57:57Z

We really should write more unit tests for this stuff rather than jumping directly to functional tests. Sometimes that's not really doable with the legacy code though.

epixa · 2017-05-17T12:44:27Z

src/core_plugins/getting_started/public/lib/add_setup_work.js

@@ -11,6 +11,67 @@ import {
  CREATE_INDEX_PATTERN_ROUTE
 } from './constants';

+function handleExistingIndexPatternsScenario(indexPatterns, currentRoute, config) {
+  // If index patterns exist, we're not going to show the user the Getting Starte page.


Starte should be Started

epixa · 2017-05-17T12:47:14Z

src/core_plugins/getting_started/public/lib/add_setup_work.js

+  }
+
+  // Otherwise, check if we have a default index pattern
+  let defaultIndexPattern = config.get('defaultIndex');


This defaultIndex logic doesn't seem appropriate in the getting started guide, as it doesn't seem to be related at all. If someone were to disable the getting_started plugin entirely, it would break the default index behavior.

I see now that this already existed in the getting started guide from the previous PR. Where was it moved from? Can it be moved back?

The same sentiment applies further down in this file with the "please create a new index pattern" logic.

This doesn't need to be dealt with in this PR since you're fixing a bug here and just moving stuff around, but we really should sort it out in a follow up.

So the reason the two pieces of logic (getting started and default index patterns) have been combined is that both use RouteSetupManager.addSetupWork(fn) to do their job.

In an early implementation I had the two independent of each other but I would see the "No default index pattern" notifier on the Getting Started page. This was happening because, AFAICT, there's no way of specifying the order of execution of the two fns registered with RouteSetupManager.addSetupWork(fn). So my solution to control the order was to combine the two pieces of logic into one fn.

ycombinator · 2017-05-17T23:10:31Z

@epixa @spalger I've added unit tests to this PR. Ready for review again.

spalger · 2017-05-17T23:44:22Z

src/core_plugins/getting_started/public/lib/add_setup_work.js

+ * gettingStartedGateCheck, which does the actual work.
+ */
+function _gettingStartedGateCheck(Private, $injector) {


How about gettingStartedGateCheckProvider? Or just putting this inline with uiRoutes.addSetupWork()?

Yeah, I'll take the latter option. Gracias.

spalger · 2017-05-17T23:47:20Z

src/ui/public/notify/notifier.js

@@ -230,7 +230,7 @@ Notifier.config = {
  errorLifetime: 300000,
  warningLifetime: 10000,
  infoLifetime: 5000,
-  setInterval: window.setInterval,
+  setInterval: window.setInterval.bind(window),


Why does setInterval need to be bound to window but clearInterval doesn't?

clearInterval probably does too. The code path I was testing with the Getting Started page ended up exercising setInterval, so I fixed that one. I will fix clearInterval as well. Thanks for pointing it out.

… multiple times

ycombinator · 2017-05-19T12:09:59Z

@spalger I've rebased on master and incorporated the changes from #11881. Mind taking a (hopefully quick) look again at this PR to make sure its working as you expect with those changes?

epixa · 2017-05-23T18:38:58Z

LGTM

spalger

LGTM

* Consolide chrome visibility toggle code into single module * Extracting code into helper functions for better readability * Removing redundant setting * Extracting functions to higher level in scope so they are not defined multiple times * Fixing typo in comment * Starting to add unit tests * Actually adding the tests this time * Adding unit tests for handleGettingStartedOptedOutScenario function * Binding this to window to prevent Uncaught TypeError: Illegal invocation * Adding unit tests for handleExistingIndexPatternsScenario function * Add helper function to aid unit testing * Refactor unit tests * Inlining function to prevent naming awkwardness * Binding this to window to remove ambiguity * Adding missing import * Fixing import * Catching expected error

ycombinator · 2017-05-23T18:54:22Z

Backported to:

5.x / 5.5: 0a88700

* Consolide chrome visibility toggle code into single module * Extracting code into helper functions for better readability * Removing redundant setting * Extracting functions to higher level in scope so they are not defined multiple times * Fixing typo in comment * Starting to add unit tests * Actually adding the tests this time * Adding unit tests for handleGettingStartedOptedOutScenario function * Binding this to window to prevent Uncaught TypeError: Illegal invocation * Adding unit tests for handleExistingIndexPatternsScenario function * Add helper function to aid unit testing * Refactor unit tests * Inlining function to prevent naming awkwardness * Binding this to window to remove ambiguity * Adding missing import * Fixing import * Catching expected error

This reverts commit c4b3ade.

* Revert "When on an embedded page, bypass Getting Started gate check (#12040)" This reverts commit 05293f1. * Revert "Making tweaks. (#12003)" This reverts commit aa3fa06. * Revert "Functional tests for the Getting Started page (#11850)" This reverts commit 099178a. * Revert "Prevent flicker on Getting Started page (#11826)" This reverts commit c4b3ade. * Revert "Getting Started page (#11805)" This reverts commit 32eff37. * Remove check for Getting Started page from navigateToApp

* Revert "When on an embedded page, bypass Getting Started gate check (elastic#12040)" This reverts commit 05293f1. * Revert "Making tweaks. (elastic#12003)" This reverts commit aa3fa06. * Revert "Functional tests for the Getting Started page (elastic#11850)" This reverts commit 099178a. * Revert "Prevent flicker on Getting Started page (elastic#11826)" This reverts commit c4b3ade. * Revert "Getting Started page (elastic#11805)" This reverts commit 32eff37. * Remove check for Getting Started page from navigateToApp

ycombinator requested review from epixa and spalger May 16, 2017 19:43

ycombinator added :Management review v5.5.0 v6.0.0 bug Fixes for quality problems that affect the customer experience labels May 16, 2017

spalger suggested changes May 16, 2017

View reviewed changes

epixa suggested changes May 17, 2017

View reviewed changes

epixa added the v6.0.0-alpha2 label May 17, 2017

spalger reviewed May 18, 2017

View reviewed changes

spalger approved these changes May 18, 2017

View reviewed changes

ycombinator added 15 commits May 19, 2017 04:43

Consolide chrome visibility toggle code into single module

2e570e3

Extracting code into helper functions for better readability

12d9dbc

Removing redundant setting

f3b6b65

Extracting functions to higher level in scope so they are not defined…

6a41995

… multiple times

Fixing typo in comment

cf3c6c8

Starting to add unit tests

1c89de6

Actually adding the tests this time

70a0a63

Adding unit tests for handleGettingStartedOptedOutScenario function

4bdc458

Binding this to window to prevent Uncaught TypeError: Illegal invocation

19f0bea

Adding unit tests for handleExistingIndexPatternsScenario function

d458195

Add helper function to aid unit testing

0350d56

Refactor unit tests

e54fa40

Inlining function to prevent naming awkwardness

65148b3

Binding this to window to remove ambiguity

965bd0b

Adding missing import

f39599d

ycombinator force-pushed the getting-started/prevent-flicker branch from 3f61821 to f39599d Compare May 19, 2017 11:48

Fixing import

fbe8c60

Catching expected error

3b418b7

epixa approved these changes May 23, 2017

View reviewed changes

spalger approved these changes May 23, 2017

View reviewed changes

ycombinator merged commit c4b3ade into elastic:master May 23, 2017

ycombinator deleted the getting-started/prevent-flicker branch May 23, 2017 18:54

ycombinator added a commit to ycombinator/kibana that referenced this pull request Jun 2, 2017

Revert "Prevent flicker on Getting Started page (elastic#11826)"

e7a20ee

This reverts commit c4b3ade.

ycombinator mentioned this pull request Jun 2, 2017

Revert the Getting Started page #12154

Merged

ycombinator added a commit to ycombinator/kibana that referenced this pull request Jun 3, 2017

Revert "Prevent flicker on Getting Started page (elastic#11826)"

6debe7f

This reverts commit c4b3ade.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent flicker on Getting Started page #11826

Prevent flicker on Getting Started page #11826

ycombinator commented May 16, 2017 •

edited

Loading

spalger left a comment

ycombinator commented May 17, 2017

epixa commented May 17, 2017

epixa May 17, 2017

epixa May 17, 2017

epixa May 17, 2017

epixa May 17, 2017

epixa May 17, 2017

ycombinator May 17, 2017 •

edited

Loading

ycombinator commented May 17, 2017

spalger May 17, 2017

ycombinator May 18, 2017

spalger May 17, 2017

ycombinator May 18, 2017

ycombinator commented May 19, 2017

epixa commented May 23, 2017

spalger left a comment

ycombinator commented May 23, 2017

Prevent flicker on Getting Started page #11826

Prevent flicker on Getting Started page #11826

Conversation

ycombinator commented May 16, 2017 • edited Loading

What's in the box

Before this PR

After this PR

spalger left a comment

Choose a reason for hiding this comment

ycombinator commented May 17, 2017

epixa commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator May 17, 2017 • edited Loading

Choose a reason for hiding this comment

ycombinator commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator commented May 19, 2017

epixa commented May 23, 2017

spalger left a comment

Choose a reason for hiding this comment

ycombinator commented May 23, 2017

ycombinator commented May 16, 2017 •

edited

Loading

ycombinator May 17, 2017 •

edited

Loading