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

[CCR] Refactor redux for Auto-follow pattern detail panel #27491

Merged
merged 10 commits into from
Dec 21, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export const updateFormErrors = (errors, existingErrors) => ({
export class AutoFollowPatternFormUI extends PureComponent {
static propTypes = {
saveAutoFollowPattern: PropTypes.func.isRequired,
onCancel: PropTypes.func,
autoFollowPattern: PropTypes.object,
apiError: PropTypes.object,
apiStatus: PropTypes.string.isRequired,
Expand Down Expand Up @@ -205,6 +206,7 @@ export class AutoFollowPatternFormUI extends PureComponent {
};

cancelForm = () => {
this.props.onCancel && this.props.onCancel();
routing.navigate('/auto_follow_patterns');
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,27 @@
import { connect } from 'react-redux';

import { SECTIONS } from '../../constants';
import { getApiStatus, getApiError, getSelectedAutoFollowPattern } from '../../store/selectors';
import { getAutoFollowPattern, saveAutoFollowPattern, clearApiError } from '../../store/actions';
import {
getApiStatus,
getApiError,
getSelectedAutoFollowPatternId,
getSelectedAutoFollowPattern,
} from '../../store/selectors';
import { getAutoFollowPattern, saveAutoFollowPattern, selectAutoFollowPattern, clearApiError } from '../../store/actions';
import { AutoFollowPatternEdit as AutoFollowPatternEditView } from './auto_follow_pattern_edit';

const scope = SECTIONS.AUTO_FOLLOW_PATTERN;

const mapStateToProps = (state) => ({
apiStatus: getApiStatus(scope)(state),
apiError: getApiError(scope)(state),
autoFollowPatternId: getSelectedAutoFollowPatternId(state),
autoFollowPattern: getSelectedAutoFollowPattern(state),
});

const mapDispatchToProps = dispatch => ({
getAutoFollowPattern: (id) => dispatch(getAutoFollowPattern(id)),
selectAutoFollowPattern: (id) => dispatch(selectAutoFollowPattern(id)),
saveAutoFollowPattern: (id, autoFollowPattern) => dispatch(saveAutoFollowPattern(id, autoFollowPattern, true)),
clearApiError: () => dispatch(clearApiError(scope)),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,40 @@ export const AutoFollowPatternEdit = injectI18n(
class extends PureComponent {
static propTypes = {
getAutoFollowPattern: PropTypes.func.isRequired,
selectAutoFollowPattern: PropTypes.func.isRequired,
saveAutoFollowPattern: PropTypes.func.isRequired,
clearApiError: PropTypes.func.isRequired,
apiError: PropTypes.object,
apiStatus: PropTypes.string.isRequired,
autoFollowPattern: PropTypes.object,
autoFollowPatternId: PropTypes.string,
}

componentDidMount() {
const { autoFollowPattern, match: { params: { id } } } = this.props;
if (!autoFollowPattern) {
const decodedId = decodeURIComponent(id);
this.props.getAutoFollowPattern(decodedId);
static getDerivedStateFromProps({ autoFollowPatternId }, { lastAutoFollowPatternId }) {
if (lastAutoFollowPatternId !== autoFollowPatternId) {
return { lastAutoFollowPatternId: autoFollowPatternId };
}
}

state = { lastAutoFollowPatternId: undefined }

componentDidMount() {
const { match: { params: { id } } } = this.props;
const decodedId = decodeURIComponent(id);

this.props.selectAutoFollowPattern(decodedId);

chrome.breadcrumbs.set([ MANAGEMENT_BREADCRUMB, listBreadcrumb, editBreadcrumb ]);
}

componentDidUpdate(prevProps, prevState) {
const { autoFollowPattern, getAutoFollowPattern } = this.props;
if (!autoFollowPattern && prevState.lastAutoFollowPatternId !== this.state.lastAutoFollowPatternId) {
// Fetch the auto-follow pattern on the server
getAutoFollowPattern(this.state.lastAutoFollowPatternId);
}
}

componentWillUnmount() {
this.props.clearApiError();
}
Expand Down Expand Up @@ -136,7 +154,7 @@ export const AutoFollowPatternEdit = injectI18n(
}

render() {
const { saveAutoFollowPattern, apiStatus, apiError, autoFollowPattern, intl } = this.props;
const { saveAutoFollowPattern, selectAutoFollowPattern, apiStatus, apiError, autoFollowPattern, intl } = this.props;

return (
<EuiPage>
Expand Down Expand Up @@ -193,6 +211,7 @@ export const AutoFollowPatternEdit = injectI18n(
remoteClusters={remoteClusters}
autoFollowPattern={autoFollowPattern}
saveAutoFollowPattern={saveAutoFollowPattern}
onCancel={() => selectAutoFollowPattern(null)}
/>
);
}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,36 +9,30 @@ import { connect } from 'react-redux';
import { SECTIONS } from '../../../constants';
import {
getListAutoFollowPatterns,
getSelectedAutoFollowPatternId,
getApiStatus,
getApiError,
isApiAuthorized,
isAutoFollowPatternDetailPanelOpen as isDetailPanelOpen,
} from '../../../store/selectors';
import {
loadAutoFollowPatterns,
openAutoFollowPatternDetailPanel as openDetailPanel,
closeAutoFollowPatternDetailPanel as closeDetailPanel,
selectAutoFollowPattern,
} from '../../../store/actions';
import { AutoFollowPatternList as AutoFollowPatternListView } from './auto_follow_pattern_list';

const scope = SECTIONS.AUTO_FOLLOW_PATTERN;

const mapStateToProps = (state) => ({
autoFollowPatterns: getListAutoFollowPatterns(state),
autoFollowPatternId: getSelectedAutoFollowPatternId(state),
apiStatus: getApiStatus(scope)(state),
apiError: getApiError(scope)(state),
isAuthorized: isApiAuthorized(scope)(state),
isDetailPanelOpen: isDetailPanelOpen(state),
});

const mapDispatchToProps = dispatch => ({
loadAutoFollowPatterns: (inBackground) => dispatch(loadAutoFollowPatterns(inBackground)),
openDetailPanel: (name) => {
dispatch(openDetailPanel(name));
},
closeDetailPanel: () => {
dispatch(closeDetailPanel());
},
selectAutoFollowPattern: (id) => dispatch(selectAutoFollowPattern(id)),
});

export const AutoFollowPatternList = connect(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,37 @@ export const AutoFollowPatternList = injectI18n(
class extends PureComponent {
static propTypes = {
loadAutoFollowPatterns: PropTypes.func,
selectAutoFollowPattern: PropTypes.func,
autoFollowPatterns: PropTypes.array,
apiStatus: PropTypes.string,
apiError: PropTypes.object,
openDetailPanel: PropTypes.func.isRequired,
closeDetailPanel: PropTypes.func.isRequired,
isDetailPanelOpen: PropTypes.bool,
}

static getDerivedStateFromProps({ autoFollowPatternId }, { lastAutoFollowPatternId }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a personal DX anecdote... as I read this code, I found myself having to trace the logic through three lifecycle methods: componentDidMount, getDerivedStateFromProps, and componentDidUpdate. I then had to also factor in what would happen when the user clicks a row (a new pattern is selected, triggering getDerivedStateFromProps). This took me a few minutes to wrap my head around, and then I was still left wondering why we're storing lastAutoFollowPatternId. Through experimentation by removing the logic it supports, I found that its purpose is to prevent an infinite render loop.

To be honest, I feel like this code has become more complex with this change. What is the benefit this complexity brings us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ComponentDidMount is pretty clear I think, it mounts and checks for the presence of a pattern in the query params. I understand what u say about the 2 other lifecycle methods, I will see how this could be improved.
There is never a benefit when complexity comes in 😊 I thought that using a middleware to change a URL on a single page was also a bit complex, mainly because you had to remember that it existed and that it was there that the search params were being changed. So I tried to move the logic as close as possible to the page (section) component.

if (autoFollowPatternId !== lastAutoFollowPatternId) {
return {
lastAutoFollowPatternId: autoFollowPatternId, // URL query param takes over store value
isDetailPanelOpen: !!autoFollowPatternId,
};
}
return null;
}

state = {
lastAutoFollowPatternId: undefined,
isDetailPanelOpen: false,
};

componentDidMount() {
this.props.loadAutoFollowPatterns();

// Check if there is an active auto-follow pattern in the URL query param
const { history: { location: { search } } } = this.props;
const { pattern } = extractQueryParams(search);
if (pattern) {
this.props.selectAutoFollowPattern(pattern);
}

// Interval to load auto-follow patterns in the background passing "true" to the fetch method
this.interval = setInterval(() => this.props.loadAutoFollowPatterns(true), REFRESH_RATE_MS);
}
Expand All @@ -40,25 +60,20 @@ export const AutoFollowPatternList = injectI18n(
clearInterval(this.interval);
}

componentDidUpdate() {
const {
openDetailPanel,
closeDetailPanel,
isDetailPanelOpen,
history: {
location: {
search,
},
},
} = this.props;

const { pattern: patternName } = extractQueryParams(search);

// Show deeplinked auto follow pattern whenever patterns get loaded or the URL changes.
if (patternName != null) {
openDetailPanel(patternName);
} else if (isDetailPanelOpen) {
closeDetailPanel();
componentDidUpdate(_, prevState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of _ can we use prevProps, which is more conventional? It seems unusual and potentially confusing to use _.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No prob.

const { history } = this.props;

if (this.state.lastAutoFollowPatternId !== prevState.lastAutoFollowPatternId) {
// Persist state to query params for deep link
if(!this.state.lastAutoFollowPatternId) {
history.replace({
search: '',
});
} else {
history.replace({
search: `?pattern=${encodeURIComponent(this.state.lastAutoFollowPatternId)}`,
});
}
}
}

Expand Down Expand Up @@ -102,7 +117,12 @@ export const AutoFollowPatternList = injectI18n(
}

renderList() {
const { autoFollowPatterns, apiStatus } = this.props;
const {
autoFollowPatterns,
apiStatus,
} = this.props;

const { isDetailPanelOpen } = this.state;

if (apiStatus === API_STATUS.LOADING) {
return (
Expand All @@ -118,7 +138,7 @@ export const AutoFollowPatternList = injectI18n(
return (
<Fragment>
<AutoFollowPatternTable autoFollowPatterns={autoFollowPatterns} />
<DetailPanel />
{isDetailPanelOpen && <DetailPanel />}
</Fragment>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,7 @@
import { connect } from 'react-redux';

import { SECTIONS } from '../../../../../constants';
import {
editAutoFollowPattern,
openAutoFollowPatternDetailPanel as openDetailPanel,
} from '../../../../../store/actions';
import { selectAutoFollowPattern } from '../../../../../store/actions';
import { getApiStatus } from '../../../../../store/selectors';
import { AutoFollowPatternTable as AutoFollowPatternTableComponent } from './auto_follow_pattern_table';

Expand All @@ -21,10 +18,7 @@ const mapStateToProps = (state) => ({
});

const mapDispatchToProps = (dispatch) => ({
editAutoFollowPattern: (name) => dispatch(editAutoFollowPattern(name)),
openDetailPanel: (name) => {
dispatch(openDetailPanel(name));
},
selectAutoFollowPattern: (name) => dispatch(selectAutoFollowPattern(name)),
});

export const AutoFollowPatternTable = connect(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const AutoFollowPatternTable = injectI18n(
class extends PureComponent {
static propTypes = {
autoFollowPatterns: PropTypes.array,
openDetailPanel: PropTypes.func.isRequired,
selectAutoFollowPattern: PropTypes.func.isRequired,
}

state = {
Expand Down Expand Up @@ -61,7 +61,7 @@ export const AutoFollowPatternTable = injectI18n(
};

getTableColumns() {
const { intl, editAutoFollowPattern, openDetailPanel } = this.props;
const { intl, selectAutoFollowPattern } = this.props;

return [{
field: 'name',
Expand All @@ -73,7 +73,7 @@ export const AutoFollowPatternTable = injectI18n(
truncateText: false,
render: (name) => {
return (
<EuiLink onClick={() => openDetailPanel(name)}>
<EuiLink onClick={() => selectAutoFollowPattern(name)}>
{name}
</EuiLink>
);
Expand Down Expand Up @@ -142,20 +142,25 @@ export const AutoFollowPatternTable = injectI18n(
},
},
{
name: intl.formatMessage({
id: 'xpack.crossClusterReplication.editIndexPattern.fields.table.actionEditLabel',
defaultMessage: 'Edit',
}),
description: intl.formatMessage({
id: 'xpack.crossClusterReplication.editIndexPattern.fields.table.actionEditDescription',
defaultMessage: 'Edit',
}),
icon: 'pencil',
onClick: ({ name }) => {
editAutoFollowPattern(name);
routing.navigate(encodeURI(`/auto_follow_patterns/edit/${encodeURIComponent(name)}`));
render: ({ name }) => {
const label = i18n.translate('xpack.crossClusterReplication.autoFollowPatternList.table.actionEditDescription', {
defaultMessage: 'Edit auto-follow pattern',
});

return (
<EuiToolTip
content={label}
delay="long"
>
<EuiButtonIcon
aria-label={label}
iconType="pencil"
color="primary"
href={routing.getAutoFollowPatternPath(name)}
/>
</EuiToolTip>
);
},
type: 'icon',
},
],
width: '100px',
Expand Down
Loading