-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Prevent flicker on Getting Started page #11826
Merged
ycombinator
merged 17 commits into
elastic:master
from
ycombinator:getting-started/prevent-flicker
May 23, 2017
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
2e570e3
Consolide chrome visibility toggle code into single module
ycombinator 12d9dbc
Extracting code into helper functions for better readability
ycombinator f3b6b65
Removing redundant setting
ycombinator 6a41995
Extracting functions to higher level in scope so they are not defined…
ycombinator cf3c6c8
Fixing typo in comment
ycombinator 1c89de6
Starting to add unit tests
ycombinator 70a0a63
Actually adding the tests this time
ycombinator 4bdc458
Adding unit tests for handleGettingStartedOptedOutScenario function
ycombinator 19f0bea
Binding this to window to prevent Uncaught TypeError: Illegal invocation
ycombinator d458195
Adding unit tests for handleExistingIndexPatternsScenario function
ycombinator 0350d56
Add helper function to aid unit testing
ycombinator e54fa40
Refactor unit tests
ycombinator 65148b3
Inlining function to prevent naming awkwardness
ycombinator 965bd0b
Binding this to window to remove ambiguity
ycombinator f39599d
Adding missing import
ycombinator fbe8c60
Fixing import
ycombinator 3b418b7
Catching expected error
ycombinator File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
179 changes: 179 additions & 0 deletions
179
src/core_plugins/getting_started/public/lib/__tests__/add_setup_work.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,179 @@ | ||
import expect from 'expect.js'; | ||
import sinon from 'sinon'; | ||
import { set } from 'lodash'; | ||
import uiChrome from 'ui/chrome'; | ||
import { Notifier } from 'ui/notify/notifier'; | ||
import { WAIT_FOR_URL_CHANGE_TOKEN } from 'ui/routes'; | ||
import { gettingStartedGateCheck } from '../add_setup_work'; | ||
import { | ||
GETTING_STARTED_ROUTE, | ||
CREATE_INDEX_PATTERN_ROUTE | ||
} from '../constants'; | ||
import { | ||
hasOptedOutOfGettingStarted, | ||
undoOptOutOfGettingStarted | ||
} from 'ui/getting_started/opt_out_helpers'; | ||
|
||
describe('Getting Started page', () => { | ||
describe('add_setup_work', () => { | ||
describe('gettingStartedGateCheck', () => { | ||
|
||
let getIds; | ||
let kbnUrl; | ||
let config; | ||
let $route; | ||
|
||
beforeEach(() => { | ||
kbnUrl = { | ||
change: sinon.spy() | ||
}; | ||
$route = {}; | ||
set($route, 'current.$$route', {}); | ||
}); | ||
|
||
describe('if index patterns exist', () => { | ||
beforeEach(() => { | ||
config = { | ||
get: sinon.stub(), | ||
set: sinon.spy() | ||
}; | ||
getIds = sinon.stub() | ||
.returns(Promise.resolve([ 'logstash-*', 'cars' ])); | ||
}); | ||
|
||
it('sets the chrome to visible', async () => { | ||
await gettingStartedGateCheck(getIds, kbnUrl, config, $route); | ||
expect(uiChrome.getVisible()).to.be(true); | ||
}); | ||
|
||
it('opts the user out of the Getting Started page', async () => { | ||
await gettingStartedGateCheck(getIds, kbnUrl, config, $route); | ||
expect(hasOptedOutOfGettingStarted()).to.be(true); | ||
}); | ||
|
||
describe('if the current route does not require a default index pattern', () => { | ||
beforeEach(() => { | ||
$route.current.$$route.requireDefaultIndex = false; | ||
}); | ||
|
||
it('returns without performing any default index pattern checks', async () => { | ||
await gettingStartedGateCheck(getIds, kbnUrl, config, $route); | ||
expect(config.get.called).to.be(false); | ||
expect(config.set.called).to.be(false); | ||
}); | ||
}); | ||
|
||
describe('if the current route requires a default index pattern', () => { | ||
beforeEach(() => { | ||
set($route, 'current.$$route.requireDefaultIndex', true); | ||
}); | ||
|
||
describe('if a default index pattern exists', () => { | ||
beforeEach(() => { | ||
config.get | ||
.withArgs('defaultIndex') | ||
.returns('an-index-pattern'); | ||
}); | ||
|
||
it('returns without setting a default index pattern', async () => { | ||
await gettingStartedGateCheck(getIds, kbnUrl, config, $route); | ||
expect(config.set.called).to.be(false); | ||
}); | ||
}); | ||
|
||
describe('if a default index pattern does not exist', () => { | ||
beforeEach(() => { | ||
config.get | ||
.withArgs('defaultIndex') | ||
.returns(undefined); | ||
}); | ||
|
||
it('sets the first index pattern as the default index pattern', async () => { | ||
await gettingStartedGateCheck(getIds, kbnUrl, config, $route); | ||
expect(config.set.calledWith('defaultIndex', 'logstash-*')).to.be(true); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('if no index patterns exist', () => { | ||
beforeEach(() => { | ||
getIds = sinon.stub() | ||
.returns(Promise.resolve([])); | ||
}); | ||
|
||
describe('if user has opted out of the Getting Started page', () => { | ||
it('sets the chrome to visible', async () => { | ||
await gettingStartedGateCheck(getIds, kbnUrl, config, $route); | ||
expect(uiChrome.getVisible()).to.be(true); | ||
}); | ||
|
||
describe('if the current route does not require a default index pattern', () => { | ||
beforeEach(() => { | ||
$route.current.$$route.requireDefaultIndex = false; | ||
}); | ||
|
||
it('returns without redirecting the user', async () => { | ||
await gettingStartedGateCheck(getIds, kbnUrl, config, $route); | ||
expect(kbnUrl.change.called).to.be(false); | ||
}); | ||
}); | ||
|
||
describe('if the current route requires a default index pattern', () => { | ||
beforeEach(() => { | ||
$route.current.$$route.requireDefaultIndex = true; | ||
}); | ||
|
||
afterEach(() => { | ||
// Clear out any notifications | ||
Notifier.prototype._notifs.length = 0; | ||
}); | ||
|
||
it('redirects the user to the Create Index Pattern page', async () => { | ||
try { | ||
await gettingStartedGateCheck(getIds, kbnUrl, config, $route); | ||
} catch (e) { | ||
expect(e).to.be(WAIT_FOR_URL_CHANGE_TOKEN); | ||
} | ||
expect(kbnUrl.change.calledWith(CREATE_INDEX_PATTERN_ROUTE)).to.be(true); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('if the user has not opted out of the Getting Started page', () => { | ||
beforeEach(() => { | ||
undoOptOutOfGettingStarted(); | ||
getIds = sinon.stub() | ||
.returns(Promise.resolve([])); | ||
}); | ||
|
||
describe('if the user is not already on Getting Started page', () => { | ||
beforeEach(() => { | ||
$route.current.$$route.originalPath = 'discover'; | ||
}); | ||
|
||
it('redirects the user to the Getting Started page', async () => { | ||
try { | ||
await gettingStartedGateCheck(getIds, kbnUrl, config, $route); | ||
} catch (e) { | ||
expect(e).to.be(WAIT_FOR_URL_CHANGE_TOKEN); | ||
} | ||
expect(kbnUrl.change.calledWith(GETTING_STARTED_ROUTE)).to.be(true); | ||
}); | ||
}); | ||
|
||
describe('if the user is already on Getting Started page', () => { | ||
beforeEach(() => { | ||
$route.current.$$route.originalPath = GETTING_STARTED_ROUTE; | ||
}); | ||
|
||
it('redirects the user to the Getting Started page', async () => { | ||
await gettingStartedGateCheck(getIds, kbnUrl, config, $route); | ||
expect(kbnUrl.change.called).to.be(false); | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,16 @@ | ||
import uiChrome from 'ui/chrome'; | ||
import { GETTING_STARTED_OPT_OUT_FLAG } from './constants'; | ||
|
||
export function hasOptedOutOfGettingStarted() { | ||
return window.localStorage.getItem(GETTING_STARTED_OPT_OUT_FLAG) || false; | ||
return Boolean(window.localStorage.getItem(GETTING_STARTED_OPT_OUT_FLAG)); | ||
} | ||
|
||
export function optOutOfGettingStarted() { | ||
window.localStorage.setItem(GETTING_STARTED_OPT_OUT_FLAG, true); | ||
uiChrome.setVisible(true); | ||
} | ||
|
||
/** | ||
* This function is intended for unit testing | ||
*/ | ||
export function undoOptOutOfGettingStarted() { | ||
window.localStorage.removeItem(GETTING_STARTED_OPT_OUT_FLAG); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same sentiment applies further down in this file with the "please create a new index pattern" logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
fn
s registered withRouteSetupManager.addSetupWork(fn)
. So my solution to control the order was to combine the two pieces of logic into onefn
.