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

Introduce redux state management for react portion of dashboard #14199

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
46e57c5
Introduce redux state management for react portion of dashboard
stacey-gammon Sep 27, 2017
11cad7f
Use redux-actions and redux-thunks to reduce boilerplate
stacey-gammon Oct 3, 2017
461c4ed
Make sure all panels are minimized from the start when a dashboard is…
stacey-gammon Oct 5, 2017
63d3e2f
Remove unused file
stacey-gammon Oct 6, 2017
338be97
use classnames dependency instead of manual logic
stacey-gammon Oct 6, 2017
11b9b37
First pass on selectors, handleActions, and more segmented reducers.
stacey-gammon Oct 6, 2017
e1d095f
Merge branch 'master' of https://github.com/elastic/kibana into chore…
stacey-gammon Oct 9, 2017
beae05e
Fix bugs with selectors and reducers and add tests that would have ca…
stacey-gammon Oct 9, 2017
abae5fb
merge with master
stacey-gammon Oct 11, 2017
544875c
Merge branch 'master' of https://github.com/elastic/kibana into chore…
stacey-gammon Oct 13, 2017
d77c0ee
Fix issue plus tests
stacey-gammon Oct 13, 2017
e7bf212
Make expanding a panel purely a css modification which avoids all re-…
stacey-gammon Oct 13, 2017
3c20f1d
Found another bug with initial state not being set correctly on a har…
stacey-gammon Oct 13, 2017
608a099
Merge branch 'master' of https://github.com/elastic/kibana into chore…
stacey-gammon Oct 14, 2017
802c34c
Remove check for change handlers now that the event handler bug is fixed
stacey-gammon Oct 14, 2017
3f84212
rename dashboardState => dashboard for reducers and redux state tree
stacey-gammon Oct 15, 2017
c6c5a9d
Remove unnecessary top level describe in jest tests
stacey-gammon Oct 15, 2017
d20bba2
Navigate back to landing page at the end of the newly added test suite
stacey-gammon Oct 15, 2017
4e884a4
Fix lint errors
stacey-gammon Oct 17, 2017
e4de737
Stabilize flaky tests by waiting until saved object search is finishe…
stacey-gammon Oct 20, 2017
76034f8
use spread operator over object.assign
stacey-gammon Oct 20, 2017
3b3ccf9
Don't leak subscriptions to the store.
stacey-gammon Oct 20, 2017
3175061
use selectors to grab dashboard panel off state.
stacey-gammon Oct 20, 2017
fe86a5c
Remove use of getState in dispatcher to avoid circular reference and …
stacey-gammon Oct 20, 2017
025a62e
Merge with master
stacey-gammon Oct 23, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,9 @@
"react-test-renderer": "15.6.1",
"react-toggle": "3.0.1",
"reactcss": "1.0.7",
"redux": "3.0.0",
"redux-thunk": "0.1.0",
"redux": "3.7.2",
"redux-actions": "2.2.1",
"redux-thunk": "2.2.0",
"request": "2.61.0",
"resize-observer-polyfill": "1.2.1",
"rimraf": "2.4.3",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import ngMock from 'ng_mock';
import expect from 'expect.js';

import { DashboardState } from '../dashboard_state';
import { DashboardStateManager } from '../dashboard_state_manager';

describe('DashboardState', function () {
let AppState;
Expand All @@ -14,7 +14,7 @@ describe('DashboardState', function () {
const mockIndexPattern = { id: 'index1' };

function initDashboardState() {
dashboardState = new DashboardState(savedDashboard, AppState, dashboardConfig);
dashboardState = new DashboardStateManager(savedDashboard, AppState, dashboardConfig);
}

beforeEach(ngMock.module('kibana'));
Expand Down Expand Up @@ -82,20 +82,20 @@ describe('DashboardState', function () {

describe('panelIndexPatternMapping', function () {
it('registers index pattern', function () {
const state = new DashboardState(savedDashboard, AppState, dashboardConfig);
const state = new DashboardStateManager(savedDashboard, AppState, dashboardConfig);
state.registerPanelIndexPatternMap('panel1', mockIndexPattern);
expect(state.getPanelIndexPatterns().length).to.equal(1);
});

it('registers unique index patterns', function () {
const state = new DashboardState(savedDashboard, AppState, dashboardConfig);
const state = new DashboardStateManager(savedDashboard, AppState, dashboardConfig);
state.registerPanelIndexPatternMap('panel1', mockIndexPattern);
state.registerPanelIndexPatternMap('panel2', mockIndexPattern);
expect(state.getPanelIndexPatterns().length).to.equal(1);
});

it('does not register undefined index pattern for panels with no index pattern', function () {
const state = new DashboardState(savedDashboard, AppState, dashboardConfig);
const state = new DashboardStateManager(savedDashboard, AppState, dashboardConfig);
state.registerPanelIndexPatternMap('markdownPanel1', undefined);
expect(state.getPanelIndexPatterns().length).to.equal(0);
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export function getContainerApiMock(config = {}) {
const containerApiMockDefaults = {
addFilter: () => {},
getAppState: () => {},
createChildUistate: () => {},
registerPanelIndexPattern: () => {},
updatePanel: () => {}
};
return Object.assign(containerApiMockDefaults, config);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export function getEmbeddableHandlerMock(config) {
const embeddableHandlerMockDefaults = {
getEditPath: () => {},
getTitleFor: () => {},
render: jest.fn()
};
return Object.assign(embeddableHandlerMockDefaults, config);
}
15 changes: 15 additions & 0 deletions src/core_plugins/kibana/public/dashboard/actions/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export {
updateViewMode,
updateIsFullScreenMode,
minimizePanel,
maximizePanel
} from './view';

export {
updatePanel,
updatePanels,
renderEmbeddable,
embeddableRenderFinished,
embeddableRenderError,
deletePanel,
} from './panels';
51 changes: 51 additions & 0 deletions src/core_plugins/kibana/public/dashboard/actions/panels.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { createAction } from 'redux-actions';

export const deletePanel = createAction('DELETE_PANEL', panelId => panelId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required, the default function in createAction is _.indentity, so you can simply write this (and other actions like this that simply pass there parameters to the reducer) like so:

export const deletePanel = createAction('DELETE_PANEL')

The only time you need to provide the function is if you need to map multiple arguments into a single object, or otherwise do something with the action's arguments before passing the values to the reducer (as you do below).


export const embeddableRenderFinished =
createAction('EMBEDDABLE_RENDER_FINISHED', (panelId, embeddable) => ({ embeddable, panelId }));

export const embeddableRenderError =
createAction('EMBEDDABLE_RENDER_ERROR', (panelId, error) => ({ panelId, error }));

export const updatePanel = createAction('UPDATE_PANEL', panel => panel);

/**
* @param panels {Array<PanelState>}
* @return {Object}
*/
export const updatePanels = createAction('UPDATE_PANELS', panels => {
const panelsMap = {};
panels.forEach(panel => {
panelsMap[panel.panelIndex] = panel;
});
return panelsMap;
});

/**
*
* @param embeddableHandler {EmbeddableHandler}
* @param panelElement {Node}
* @param panelId {string}
* @param containerApi {ContainerAPI}
* @return {function(*, *)}
*/
export function renderEmbeddable(embeddableHandler, panelElement, panelId, containerApi) {
return (dispatch, getState) => {
const { dashboardState } = getState();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general getState should be avoided in actions (imo it should be considered an escape-hatch). My immediate thought is that the panelState below should be injected here instead of just injecting the pabelId. (Though, I haven't read through the entire code yet, so I might be lacking some context still)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely understand the sentiment, but I ran into this same pattern and the reason I wanted to stick with getState in the action is because I didn't want state changes for that particular state to cause a re-render of my component unnecessarily. So in this case, it doesn't seem like the component needs to know about that particular part of the state tree so adding it here just to make it available in the presentational component so it can call the action with it seems not great to me.

Any thoughts on that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a re-render a problem? If this state updates multiple times per second, I might agree (though, I'd likely push towards implementing a shouldComponentUpdate if performance on this component is that important and we're hitting rendering slowness). I still prefer injecting it, as I feel it makes the code clearer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing inherently wrong, but just felt off to do it that way. I definitely understand the hesitation around calling getState within the action, but I'm not (yet) convinced it's an anti pattern, but always willing to hear counter arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to agree with @chrisronline on this one, though perhaps because it makes my life easier. If the only reason your putting a prop in a component is to pass it to a function, which goes back to the container, seems like an unnecessary loop.

I was actually surprised I couldn't get the state inside of mapDispatchToProps - that would solve this. I have the whole state tree inside of mapStateToProps, why not also have it accessible in mapDispatchToProps? Unless it is and I don't know about it? Because if it was I could do something like:


const mapDispatchToProps = (dispatch, { embeddableHandler, panelId, getContainerApi }, { dashboardState }) => ({
  renderEmbeddable: (panelElement) => {
    const panel = getPanel(dashboardState, panelId);
    return renderEmbeddable(embeddableHandler, panelElement, panel, getContainerApi());
  }
}

But because I couldn't do that, I just pushed the getState inside the action.

Seems silly to make the DashboardPanelContainer require panel as a prop instead of just panelId. But, that's just my gut feel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try messing with the mergeProps discussed here but from what I remember when playing with it, it didn't stop the unnecessary rerender, but feel free to try it out

const panelState = dashboardState.panels[panelId];

if (!embeddableHandler) {
dispatch(embeddableRenderError(panelId, new Error(`Invalid embeddable type "${panelState.type}"`)));
return;
}

return embeddableHandler.render(panelElement, panelState, containerApi)
.then(embeddable => {
return dispatch(embeddableRenderFinished(panelId, embeddable));
})
.catch(error => {
dispatch(embeddableRenderError(panelId, error));
});
};
}
6 changes: 6 additions & 0 deletions src/core_plugins/kibana/public/dashboard/actions/view.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { createAction } from 'redux-actions';

export const updateViewMode = createAction('UPDATE_VIEW_MODE', viewMode => viewMode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment in the panels actions; all of these action creators don't need to the second argument.

export const maximizePanel = createAction('MAXIMIZE_PANEl', maximizedPanelId => maximizedPanelId);
export const minimizePanel = createAction('MINIMIZE_PANEL', () => {});
export const updateIsFullScreenMode = createAction('UPDATE_IS_FULL_SCREEN_MODE', isFullScreenMode => isFullScreenMode);
Original file line number Diff line number Diff line change
Expand Up @@ -77,26 +77,10 @@ <h2 class="kuiTitle kuiVerticalRhythm">
</p>
</div>

<dashboard-grid
ng-show="!hasExpandedPanel()"
panels="panels"
is-full-screen-mode="!chrome.getVisible()"
dashboard-view-mode="dashboardViewMode"
get-embeddable-handler="getEmbeddableHandler"
<dashboard-viewport-provider
get-container-api="getContainerApi"
expand-panel="expandPanel"
data-shared-items-count="{{panels.length}}"
on-panel-removed="onPanelRemoved"
></dashboard-grid>

<dashboard-panel
ng-if="hasExpandedPanel()"
panel="expandedPanel"
is-full-screen-mode="!chrome.getVisible()"
is-expanded="true"
dashboard-view-mode="dashboardViewMode"
get-embeddable-handler="getEmbeddableHandler"
get-container-api="getContainerApi"
on-toggle-expanded="minimizeExpandedPanel"
></dashboard-panel>
>
</dashboard-viewport-provider>

</dashboard-app>
Loading