-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Lens] allow user to disable auto apply #125158
Merged
drewdaemon
merged 88 commits into
elastic:main
from
drewdaemon:74490/disable-auto-apply-duplicate-state-strategy
Mar 4, 2022
Merged
Changes from 76 commits
Commits
Show all changes
88 commits
Select commit
Hold shift + click to select a range
ba8b5a5
Auto-apply toggle switch + reducers
drewdaemon 0968920
add apply changes button
drewdaemon 7bb60c6
kinda sorta workin!
drewdaemon 4c942d9
auto apply applies working changes
drewdaemon c5dd500
record applied datasource layers in frame public API
drewdaemon 49069af
Save setting to localstorage
drewdaemon 36a67e2
hide suggestions until changes applied
drewdaemon 569f82a
rename applyWorkingState -> applyChanges
drewdaemon 0d450d9
apply suggestions when clicked
drewdaemon 6e6f127
apply state changes due to data panel load immediately
drewdaemon 1add31a
consolidate applied state logic
drewdaemon 322b1d3
keep flyout open when apply changes
drewdaemon 976242e
generalizing exclusion mechanism
drewdaemon 009dee3
only update extent when it changes meaningfully
drewdaemon 3288e42
Merge branch 'main' of github.com:elastic/kibana into 74490/disable-a…
drewdaemon 1e96ab0
add reducer tests
drewdaemon aab38f1
test selectors
drewdaemon 3980e08
data-panel test (not working yet)
drewdaemon 14ba2be
test that datasource applies changes immediately
drewdaemon 026737c
test DataPanelWrapper's setState function
drewdaemon 4811574
Test suggestion panel apply changes prompt
drewdaemon dcbc4f7
Move controls to toolbar
drewdaemon 8dd20fe
some UI tweaks
drewdaemon dbcb18f
fix disabling auto apply from localstorage for new vis
drewdaemon 13b0599
correcting workspace panel regression
drewdaemon a91bd24
make button smaller to conserve space
drewdaemon 25f22c4
test that workspace panel uses applied state
drewdaemon f2fb7f3
Merge branch 'main' of github.com:elastic/kibana into 74490/disable-a…
drewdaemon cf1f3e4
test auto-apply controls
drewdaemon 62be81d
Merge branch 'main' into 74490/disable-auto-apply-duplicate-state-str…
kibanamachine e3c6416
some small tweaks
drewdaemon 89391c0
Merge branch '74490/disable-auto-apply-duplicate-state-strategy' of g…
drewdaemon 294d8e5
prevent suggestions not from suggestions panel from being applied imm…
drewdaemon 371962c
reinstate apply label
drewdaemon 126ad53
update some types
drewdaemon 4b3169b
preserve auto-apply controls for fullscreen datasource
drewdaemon d9053e6
some updates to the controls
drewdaemon 88e2be1
fix ui in fullscreen mode
drewdaemon 7a55fd6
fix excluded click target check
drewdaemon ff02fd2
stop changing visualization type from changing workspace panel contents
drewdaemon 12f2e81
Merge branch 'main' into 74490/disable-auto-apply-duplicate-state-str…
kibanamachine 7f18f65
apply changes on save
drewdaemon 271a0b1
Merge branch '74490/disable-auto-apply-duplicate-state-strategy' of g…
drewdaemon bfd4303
correct test title
drewdaemon 730a608
move apply-changes-on-save to avoid race condition
drewdaemon d269777
Merge branch 'main' into 74490/disable-auto-apply-duplicate-state-str…
kibanamachine a65d0e2
isolate expression generation
drewdaemon f7d88e7
factoring some validation into own function
drewdaemon fb47b75
select active datasource inside getBuildExpressionArgs
drewdaemon 3d06710
assert saveability against unapplied state
drewdaemon e4f9b4a
Merge branch '74490/disable-auto-apply-duplicate-state-strategy' of g…
drewdaemon 25a70dd
add functional tests for auto-apply in full-screen and page refresh
drewdaemon a69b9fd
functional test: apply changes
drewdaemon 77fd88f
test applying through suggestions
drewdaemon 96d77a6
passing working state to workspace panel wrapper
drewdaemon 7e4c955
caching expression strategy
drewdaemon 59d1ece
apply-changes counter
drewdaemon ff5465e
move initial render flag to local state
drewdaemon 638fd5c
fix expression check
drewdaemon 923bf57
refine workspace panel state usage
drewdaemon 6d8b1d3
unskip auto-apply test
drewdaemon 806b6d7
remove missing dependency
drewdaemon a21de00
allow null expression as initial render (empty workspace)
drewdaemon cf332d6
add empty workspace test
drewdaemon 93f444a
Merge branch 'main' into 74490/disable-auto-apply-duplicate-state-str…
kibanamachine 1d762e1
base saveability on working changes
drewdaemon 4c39e45
mark changes as applied when auto-apply is enabled
drewdaemon 3fb4390
update selectors test
drewdaemon eada3fe
encapsulate apply-changes trigger
drewdaemon ababa8e
update unit tests
drewdaemon 25b520b
Add telemetry
drewdaemon c676959
Merge branch '74490/disable-auto-apply-duplicate-state-strategy' of g…
drewdaemon 1031b2a
Merge pull request #4 from andrewctate/74490/disable-auto-apply-toggl…
drewdaemon 73eb511
Merge branch 'main' of github.com:elastic/kibana into 74490/disable-a…
drewdaemon acbd930
fix typo in telemetry configs
drewdaemon d966510
fix typings
drewdaemon b540de4
remove unused variable
drewdaemon 8a850f0
useRef for expressionToRender
drewdaemon fba80cb
stop context middleware from updating time range when user has unappl…
drewdaemon d6ba1db
update context-update behavior when auto-apply disabled
drewdaemon a5493e6
fix typings
drewdaemon 6a58a86
wrapping changesApplied signal in useEffect
drewdaemon 9b0e750
using ref for marking initial render complete
drewdaemon 41d8e8e
handle expressionToRender in an effect hook
drewdaemon 80f9f1e
fix typings
drewdaemon be329a3
Merge branch 'main' into 74490/disable-auto-apply-duplicate-state-str…
kibanamachine 381ec60
allow falsy value for axis title
drewdaemon 74a3dbd
Merge branch '74490/disable-auto-apply-duplicate-state-strategy' of g…
drewdaemon 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
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
87 changes: 87 additions & 0 deletions
87
x-pack/plugins/lens/public/editor_frame_service/editor_frame/data_panel_wrapper.test.tsx
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,87 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import React from 'react'; | ||
import { DataPanelWrapper } from './data_panel_wrapper'; | ||
import { Datasource, DatasourceDataPanelProps } from '../../types'; | ||
import { DragDropIdentifier } from '../../drag_drop'; | ||
import { UiActionsStart } from 'src/plugins/ui_actions/public'; | ||
import { mockStoreDeps, mountWithProvider } from '../../mocks'; | ||
import { disableAutoApply } from '../../state_management/lens_slice'; | ||
import { selectTriggerApplyChanges } from '../../state_management'; | ||
|
||
describe('Data Panel Wrapper', () => { | ||
describe('Datasource data panel properties', () => { | ||
let datasourceDataPanelProps: DatasourceDataPanelProps; | ||
let lensStore: Awaited<ReturnType<typeof mountWithProvider>>['lensStore']; | ||
beforeEach(async () => { | ||
const renderDataPanel = jest.fn(); | ||
|
||
const datasourceMap = { | ||
activeDatasource: { | ||
renderDataPanel, | ||
} as unknown as Datasource, | ||
}; | ||
|
||
const mountResult = await mountWithProvider( | ||
<DataPanelWrapper | ||
datasourceMap={datasourceMap} | ||
showNoDataPopover={() => {}} | ||
core={{} as DatasourceDataPanelProps['core']} | ||
dropOntoWorkspace={(field: DragDropIdentifier) => {}} | ||
hasSuggestionForField={(field: DragDropIdentifier) => true} | ||
plugins={{ uiActions: {} as UiActionsStart }} | ||
/>, | ||
{ | ||
preloadedState: { | ||
activeDatasourceId: 'activeDatasource', | ||
datasourceStates: { | ||
activeDatasource: { | ||
isLoading: false, | ||
state: { | ||
age: 'old', | ||
}, | ||
}, | ||
}, | ||
}, | ||
storeDeps: mockStoreDeps({ datasourceMap }), | ||
} | ||
); | ||
|
||
lensStore = mountResult.lensStore; | ||
|
||
datasourceDataPanelProps = renderDataPanel.mock.calls[0][1] as DatasourceDataPanelProps; | ||
}); | ||
|
||
describe('setState', () => { | ||
it('applies state immediately when option true', async () => { | ||
lensStore.dispatch(disableAutoApply()); | ||
selectTriggerApplyChanges(lensStore.getState()); | ||
|
||
const newDatasourceState = { age: 'new' }; | ||
datasourceDataPanelProps.setState(newDatasourceState, { applyImmediately: true }); | ||
|
||
expect(lensStore.getState().lens.datasourceStates.activeDatasource.state).toEqual( | ||
newDatasourceState | ||
); | ||
expect(selectTriggerApplyChanges(lensStore.getState())).toBeTruthy(); | ||
}); | ||
|
||
it('does not apply state immediately when option false', async () => { | ||
lensStore.dispatch(disableAutoApply()); | ||
selectTriggerApplyChanges(lensStore.getState()); | ||
|
||
const newDatasourceState = { age: 'new' }; | ||
datasourceDataPanelProps.setState(newDatasourceState, { applyImmediately: false }); | ||
|
||
const lensState = lensStore.getState().lens; | ||
expect(lensState.datasourceStates.activeDatasource.state).toEqual(newDatasourceState); | ||
expect(selectTriggerApplyChanges(lensStore.getState())).toBeFalsy(); | ||
}); | ||
}); | ||
}); | ||
}); |
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
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
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 |
---|---|---|
|
@@ -88,3 +88,7 @@ | |
text-align: center; | ||
flex-grow: 0; | ||
} | ||
|
||
.lnsSuggestionPanel__applyChangesPrompt { | ||
height: $lnsSuggestionHeight; | ||
} |
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.
I am wondering if it wouldn't be more performant if instead of dispatching 2 actions one after another, we could add a parameter
options
toupdateDatasourceState
? So we'd invoke it in this wayand then we'd incorporate the logic of
applyChanges
action insideupdateDatasourceState
. Same for other places when we runapplyChanges
right after running another actions (I think there are 3 in this PR) I didn't test it though so not sure if that would work ok. WDYT?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.
Thanks for the feedback! I went the direction of two dispatches to keep the reducers simple and atomic. Can you help me understand in what way using a single
dispatch
call would be more performant?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 main my concern is that we dispatch one more action so we have one more rerender of the app. I might be wrong though, I might test it on Monday :D Here's a random article that explains it: https://unsplash.com/blog/react-redux-performance-considerations-when-dispatching-multiple-actions/
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.
Did you get a chance to test this? Not your responsibility, just wanted to check if it has been done.