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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Dec 21, 2018
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
43db32e
[CCR] Refactor redux for Auto-follow pattern detail panel
sebelga Dec 19, 2018
a7e764a
[CCR] Small refactor
sebelga Dec 19, 2018
d173c56
[CCR] Change to present tense
sebelga Dec 20, 2018
f5df213
[CCR] Display auto-follow pattern name even if it does not exist
sebelga Dec 20, 2018
ae7947e
[CCR] Use href to edit auto-follow pattern + remove middelware to upd…
sebelga Dec 20, 2018
e33a410
Merge branch 'feature/ccr' into feature/ccr_refactor_redux
sebelga Dec 20, 2018
591a536
[CCR] Fix navigation back bug + set 2 ids for detail and edit an auto…
sebelga Dec 21, 2018
0c4af91
[CCR] Replace api middleware with redux-thunk action
sebelga Dec 21, 2018
dd4fcbb
Merge branch 'feature/ccr' into feature/ccr_refactor_redux
sebelga Dec 21, 2018
3d0f446
[CCR] Show detail footer close button even when cluster is not valid
sebelga Dec 21, 2018
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
19 changes: 9 additions & 10 deletions ...n/public/app/sections/home/auto_follow_pattern_list/auto_follow_pattern_list.container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,36 +9,35 @@ import { connect } from 'react-redux';
import { SECTIONS } from '../../../constants';
import {
getListAutoFollowPatterns,
getSelectedAutoFollowPatternId,
getApiStatus,
getApiError,
isApiAuthorized,
isAutoFollowPatternDetailPanelOpen as isDetailPanelOpen,
isAutoFollowPatternDetailPanelOpened as isDetailPanelOpened,
} from '../../../store/selectors';
import {
loadAutoFollowPatterns,
openAutoFollowPatternDetailPanel as openDetailPanel,
closeAutoFollowPatternDetailPanel as closeDetailPanel,
selectAutoFollowPattern,
toggleAutoFollowPatternDetailPanel,
} 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),
isDetailPanelOpened: isDetailPanelOpened(state),
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, but I think we should use the present tense in all booleans for consistency, unless the tense has some clear significance.

Copy link
Contributor Author

@sebelga sebelga Dec 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ok! Was not sure if it was a mistake. So you prefer isClose to isClosed for example? I am not native but it sounds weird to me 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

English is such a terrible language... "closed" is both the present and past tense, so complementary states would be isClosed and isOpen.

});

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

export const AutoFollowPatternList = connect(
Expand Down
41 changes: 27 additions & 14 deletions ...replication/public/app/sections/home/auto_follow_pattern_list/auto_follow_pattern_list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@ 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,
isDetailPanelOpened: PropTypes.bool,
}

componentDidMount() {
Expand All @@ -41,24 +42,32 @@ export const AutoFollowPatternList = injectI18n(
}

componentDidUpdate() {
const { history: { location: { search } } } = this.props;
let { pattern } = extractQueryParams(search);

pattern = !!pattern ? pattern : null;
this.onAutoFollowPatternChange(pattern);
}

onAutoFollowPatternChange(newId) {
const {
autoFollowPatternId,
selectAutoFollowPattern,
openDetailPanel,
closeDetailPanel,
isDetailPanelOpen,
history: {
location: {
search,
},
},
} = this.props;

const { pattern: patternName } = extractQueryParams(search);
if (newId !== autoFollowPatternId) {
selectAutoFollowPattern(newId);
}

// Show deeplinked auto follow pattern whenever patterns get loaded or the URL changes.
if (patternName != null) {
openDetailPanel(patternName);
} else if (isDetailPanelOpen) {
/**
* Single source of truth to open or close the detail panel
*/
if (!newId) {
closeDetailPanel();
} else {
openDetailPanel();
}
}

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

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

if (apiStatus === API_STATUS.LOADING) {
return (
Expand All @@ -118,7 +131,7 @@ export const AutoFollowPatternList = injectI18n(
return (
<Fragment>
<AutoFollowPatternTable autoFollowPatterns={autoFollowPatterns} />
<DetailPanel />
{isDetailPanelOpened && <DetailPanel />}
</Fragment>
);
}
Expand Down
10 changes: 2 additions & 8 deletions ..._pattern_list/components/auto_follow_pattern_table/auto_follow_pattern_table.container.js
View file
Open in desktop
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
8 changes: 4 additions & 4 deletions ...uto_follow_pattern_list/components/auto_follow_pattern_table/auto_follow_pattern_table.js
View file
Open in desktop
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 @@ -152,7 +152,7 @@ export const AutoFollowPatternTable = injectI18n(
}),
icon: 'pencil',
onClick: ({ name }) => {
editAutoFollowPattern(name);
selectAutoFollowPattern(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to select the auto-follow pattern here? I just tried removing this line and the editing functionality seemed to work fine.

On a separate note, I think we may want to implement this the way we did in Remote Clusters, so that this is a link with an href instead of a button with an onClick. This way people could open this in a new tab or copy the URL if they wanted to for some reason: https://github.com/elastic/kibana/blob/master/x-pack/plugins/remote_clusters/public/sections/remote_cluster_list/remote_cluster_table/remote_cluster_table.js#L183

Copy link
Contributor Author

@sebelga sebelga Dec 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It works because, when there is no selection the edit component looks at the route param and fetches from the server (which is what happens on a page refresh). I'll change it to an href, good point

routing.navigate(encodeURI(`/auto_follow_patterns/edit/${encodeURIComponent(name)}`));
},
type: 'icon',
Expand Down
45 changes: 12 additions & 33 deletions .../sections/home/auto_follow_pattern_list/components/detail_panel/detail_panel.container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,43 +7,22 @@
import { connect } from 'react-redux';
import { DetailPanel as DetailPanelView } from './detail_panel';

import {
getDetailPanelAutoFollowPattern,
getDetailPanelAutoFollowPatternName,
getApiStatus,
isAutoFollowPatternDetailPanelOpen as isDetailPanelOpen,
} from '../../../../../store/selectors';

import {
closeAutoFollowPatternDetailPanel as closeDetailPanel,
editAutoFollowPattern,
} from '../../../../../store/actions';

import {
SECTIONS
} from '../../../../../constants';
import { getSelectedAutoFollowPattern, getApiStatus, } from '../../../../../store/selectors';
import { selectAutoFollowPattern } from '../../../../../store/actions';
import { SECTIONS } from '../../../../../constants';

const scope = SECTIONS.AUTO_FOLLOW_PATTERN;

const mapStateToProps = (state) => {
return {
isDetailPanelOpen: isDetailPanelOpen(state),
autoFollowPattern: getDetailPanelAutoFollowPattern(state),
autoFollowPatternName: getDetailPanelAutoFollowPatternName(state),
apiStatus: getApiStatus(scope)(state),
};
};
const mapStateToProps = (state) => ({
autoFollowPattern: getSelectedAutoFollowPattern(state),
apiStatus: getApiStatus(scope)(state),
});

const mapDispatchToProps = (dispatch) => {
return {
closeDetailPanel: () => {
dispatch(closeDetailPanel());
},
editAutoFollowPattern: (name) => {
dispatch(editAutoFollowPattern(name));
}
};
};
const mapDispatchToProps = (dispatch) => ({
// The actual closing of the panel is done in the <AutoFollowPatternList />
// component as it listens to the URL query change
closeDetailPanel: () => dispatch(selectAutoFollowPattern(null)),
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic might be a little easier to follow if the owner (AutoFollowPatternList) were to provide this callback instead of the container. Then the comment becomes a little less necessary because all of the related logic is already in front of you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point

});

export const DetailPanel = connect(
mapStateToProps,
Expand Down
31 changes: 10 additions & 21 deletions ...public/app/sections/home/auto_follow_pattern_list/components/detail_panel/detail_panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,9 @@ import routing from '../../../../../services/routing';

export class DetailPanelUi extends Component {
static propTypes = {
isDetailPanelOpen: PropTypes.bool.isRequired,
apiStatus: PropTypes.string,
autoFollowPattern: PropTypes.object,
autoFollowPatternName: PropTypes.string,
closeDetailPanel: PropTypes.func.isRequired,
editAutoFollowPattern: PropTypes.func.isRequired,
}

renderAutoFollowPattern() {
Expand Down Expand Up @@ -251,16 +248,16 @@ export class DetailPanelUi extends Component {

renderFooter() {
const {
editAutoFollowPattern,
autoFollowPattern,
autoFollowPatternName,
closeDetailPanel,
} = this.props;

if (!autoFollowPattern) {
return null;
}

const { name: autoFollowPatternName } = autoFollowPattern;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the auto-follow pattern doesn't exist, then the name will be undefined. Compare this to Remote Clusters, which will still show you the name if the remote cluster can't be found.

image

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch


return (
<EuiFlyoutFooter>
<EuiFlexGroup justifyContent="spaceBetween" alignItems="center">
Expand Down Expand Up @@ -300,7 +297,6 @@ export class DetailPanelUi extends Component {
fill
color="primary"
onClick={() => {
editAutoFollowPattern(autoFollowPatternName);
routing.navigate(encodeURI(`/auto_follow_patterns/edit/${encodeURIComponent(autoFollowPatternName)}`));
}}
>
Expand All @@ -318,15 +314,7 @@ export class DetailPanelUi extends Component {
}

render() {
const {
isDetailPanelOpen,
closeDetailPanel,
autoFollowPatternName,
} = this.props;

if (!isDetailPanelOpen) {
return null;
}
const { autoFollowPattern, closeDetailPanel } = this.props;

return (
<EuiFlyout
Expand All @@ -336,14 +324,15 @@ export class DetailPanelUi extends Component {
size="m"
maxWidth={400}
>
<EuiFlyoutHeader>
<EuiTitle size="m" id="autoFollowPatternDetailsFlyoutTitle">
<h2>{autoFollowPatternName}</h2>
</EuiTitle>
</EuiFlyoutHeader>
{autoFollowPattern && (
<EuiFlyoutHeader>
<EuiTitle size="m" id="autoFollowPatternDetailsFlyoutTitle">
<h2>{autoFollowPattern.name}</h2>
</EuiTitle>
</EuiFlyoutHeader>
)}

{this.renderContent()}

{this.renderFooter()}
</EuiFlyout>
);
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/cross_cluster_replication/public/app/store/action_types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ export const API_REQUEST_END = 'API_REQUEST_END';
export const API_ERROR_SET = 'API_ERROR_SET';

// Auto Follow Pattern
export const AUTO_FOLLOW_PATTERN_EDIT = 'AUTO_FOLLOW_PATTERN_EDIT';
export const AUTO_FOLLOW_PATTERN_SELECT = 'AUTO_FOLLOW_PATTERN_SELECT';
export const AUTO_FOLLOW_PATTERN_LOAD = 'AUTO_FOLLOW_PATTERN_LOAD';
export const AUTO_FOLLOW_PATTERN_GET = 'AUTO_FOLLOW_PATTERN_GET';
export const AUTO_FOLLOW_PATTERN_CREATE = 'AUTO_FOLLOW_PATTERN_CREATE';
export const AUTO_FOLLOW_PATTERN_UPDATE = 'AUTO_FOLLOW_PATTERN_UPDATE';
export const AUTO_FOLLOW_PATTERN_DELETE = 'AUTO_FOLLOW_PATTERN_DELETE';
export const AUTO_FOLLOW_PATTERN_DETAIL_PANEL = 'AUTO_FOLLOW_PATTERN_DETAIL_PANEL';
export const AUTO_FOLLOW_PATTERN_DETAIL_PANEL_TOGGLE = 'AUTO_FOLLOW_PATTERN_DETAIL_PANEL_TOGGLE';
27 changes: 12 additions & 15 deletions x-pack/plugins/cross_cluster_replication/public/app/store/actions/auto_follow_pattern.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,30 +13,26 @@ import {
updateAutoFollowPattern as updateAutoFollowPatternRequest,
deleteAutoFollowPattern as deleteAutoFollowPatternRequest,
} from '../../services/api';
import { arrify } from '../../../../common/services/utils';
import routing from '../../services/routing';
import * as t from '../action_types';
import { sendApiRequest } from './api';
import { getDetailPanelAutoFollowPatternName } from '../selectors';
import { getSelectedAutoFollowPatternId } from '../selectors';

const { AUTO_FOLLOW_PATTERN: scope } = SECTIONS;

export const editAutoFollowPattern = (name) => ({
type: t.AUTO_FOLLOW_PATTERN_EDIT,
export const selectAutoFollowPattern = (name) => ({
type: t.AUTO_FOLLOW_PATTERN_SELECT,
payload: name
});

export const openAutoFollowPatternDetailPanel = (name) => {
export const toggleAutoFollowPatternDetailPanel = (isOpen = true) => {
return {
type: t.AUTO_FOLLOW_PATTERN_DETAIL_PANEL,
payload: name
type: t.AUTO_FOLLOW_PATTERN_DETAIL_PANEL_TOGGLE,
payload: isOpen
};
};

export const closeAutoFollowPatternDetailPanel = () => ({
type: t.AUTO_FOLLOW_PATTERN_DETAIL_PANEL,
payload: null
});

export const loadAutoFollowPatterns = (isUpdating = false) =>
sendApiRequest({
label: t.AUTO_FOLLOW_PATTERN_LOAD,
Expand All @@ -54,7 +50,7 @@ export const getAutoFollowPattern = (id) =>
handler: async (dispatch) => (
getAutoFollowPatternRequest(id)
.then((response) => {
dispatch(editAutoFollowPattern(id));
dispatch(selectAutoFollowPattern(id));
return response;
})
)
Expand Down Expand Up @@ -135,9 +131,10 @@ export const deleteAutoFollowPattern = (id) => (
}

// If we've just deleted a pattern we were looking at, we need to close the panel.
const detailPanelAutoFollowPatternName = getDetailPanelAutoFollowPatternName(getState());
if (detailPanelAutoFollowPatternName && response.itemsDeleted.includes(detailPanelAutoFollowPatternName)) {
dispatch(closeAutoFollowPatternDetailPanel());
const ids = arrify(id);
const autoFollowPatternId = getSelectedAutoFollowPatternId(getState());
if (ids.some(_id => autoFollowPatternId === _id)) {
Copy link
Contributor

@cjcenizal cjcenizal Dec 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The original code anticipated that the deletion request could fail for the selected auto-follow pattern. It looks like the new code doesn't anticipate that case and will close the detail panel regardless of the request failing or succeeding. Is this correct? If so, then I think the first case makes more sense from the user's perspective. What do you think?

Copy link
Contributor Author

@sebelga sebelga Dec 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is inside the onSuccess handler so will only be executed if the deletion succeeded. But it made me realize that I should change the if statement to if (ids.includes(autoFollowPatternId)) 😊

dispatch(selectAutoFollowPattern(null));
}
}
})
Expand Down
Loading