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] Implement Advanced Settings design feedback #29543

Merged
merged 9 commits into from
Jan 30, 2019
Merged

[CCR] Implement Advanced Settings design feedback #29543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
75f3067
Use EuiSwitch to toggle advanced settings in Create Follower Index form.
cjcenizal Jan 29, 2019
a77511a
Move 'optional' from each Advanced Setting field to the section heading.
cjcenizal Jan 29, 2019
5325fa6
Change Advanced Settings switch label and description to emphasize th…
cjcenizal Jan 29, 2019
b859d4b
Prepopulate Advanced Settings fields with default values.
cjcenizal Jan 29, 2019
6c05b6e
When editing a follower index, check if advanced settings have been e…
cjcenizal Jan 29, 2019
dd191e2
Add 'Reset to default' button below advanced settings fields if their…
cjcenizal Jan 29, 2019
70bd18e
Remove 'Default' copy from Advanced Settings descriptions.
cjcenizal Jan 30, 2019
022ea8a
Remove 'Reset to default' buttons from fields which don't have a defa…
cjcenizal Jan 30, 2019
433379b
Simplify toggleAdvancedSettings function, add comments, and fix React…
cjcenizal Jan 30, 2019
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
30 changes: 19 additions & 11 deletions ...cluster_replication/public/app/components/follower_index_form/advanced_settings_fields.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export const advancedSettingsFields = [
),
label: i18n.translate(
'xpack.crossClusterReplication.followerIndexForm.advancedSettings.maxReadRequestOperationCountLabel', {
defaultMessage: 'Max read request operation count (optional)'
defaultMessage: 'Max read request operation count'
}
),
defaultValue: getSettingDefault('maxReadRequestOperationCount'),
Expand All @@ -72,7 +72,7 @@ export const advancedSettingsFields = [
}),
label: i18n.translate(
'xpack.crossClusterReplication.followerIndexForm.advancedSettings.maxOutstandingReadRequestsLabel', {
defaultMessage: 'Max outstanding read requests (optional)'
defaultMessage: 'Max outstanding read requests'
}
),
defaultValue: getSettingDefault('maxOutstandingReadRequests'),
Expand All @@ -89,7 +89,7 @@ export const advancedSettingsFields = [
}),
label: i18n.translate(
'xpack.crossClusterReplication.followerIndexForm.advancedSettings.maxReadRequestSizeLabel', {
defaultMessage: 'Max read request size (optional)'
defaultMessage: 'Max read request size'
}
),
defaultValue: getSettingDefault('maxReadRequestSize'),
Expand All @@ -108,7 +108,7 @@ export const advancedSettingsFields = [
),
label: i18n.translate(
'xpack.crossClusterReplication.followerIndexForm.advancedSettings.maxWriteRequestOperationCountLabel', {
defaultMessage: 'Max write request operation count (optional)'
defaultMessage: 'Max write request operation count'
}
),
defaultValue: getSettingDefault('maxWriteRequestOperationCount'),
Expand All @@ -127,7 +127,7 @@ export const advancedSettingsFields = [
),
label: i18n.translate(
'xpack.crossClusterReplication.followerIndexForm.advancedSettings.maxWriteRequestSizeLabel', {
defaultMessage: 'Max write request size (optional)'
defaultMessage: 'Max write request size'
}
),
defaultValue: getSettingDefault('maxWriteRequestSize'),
Expand All @@ -146,7 +146,7 @@ export const advancedSettingsFields = [
),
label: i18n.translate(
'xpack.crossClusterReplication.followerIndexForm.advancedSettings.maxOutstandingWriteRequestsLabel', {
defaultMessage: 'Max outstanding write requests (optional)'
defaultMessage: 'Max outstanding write requests'
}
),
defaultValue: getSettingDefault('maxOutstandingWriteRequests'),
Expand All @@ -167,7 +167,7 @@ export const advancedSettingsFields = [
),
label: i18n.translate(
'xpack.crossClusterReplication.followerIndexForm.advancedSettings.maxWriteBufferCountLabel', {
defaultMessage: 'Max write buffer count (optional)'
defaultMessage: 'Max write buffer count'
}
),
defaultValue: getSettingDefault('maxWriteBufferCount'),
Expand All @@ -188,7 +188,7 @@ export const advancedSettingsFields = [
),
label: i18n.translate(
'xpack.crossClusterReplication.followerIndexForm.advancedSettings.maxWriteBufferSizeLabel', {
defaultMessage: 'Max write buffer size (optional)'
defaultMessage: 'Max write buffer size'
}
),
defaultValue: getSettingDefault('maxWriteBufferSize'),
Expand All @@ -208,7 +208,7 @@ export const advancedSettingsFields = [
),
label: i18n.translate(
'xpack.crossClusterReplication.followerIndexForm.advancedSettings.maxRetryDelayLabel', {
defaultMessage: 'Max retry delay (optional)'
defaultMessage: 'Max retry delay'
}
),
defaultValue: getSettingDefault('maxRetryDelay'),
Expand All @@ -230,7 +230,7 @@ export const advancedSettingsFields = [
),
label: i18n.translate(
'xpack.crossClusterReplication.followerIndexForm.advancedSettings.readPollTimeoutLabel', {
defaultMessage: 'Read poll timeout (optional)'
defaultMessage: 'Read poll timeout'
}
),
defaultValue: getSettingDefault('readPollTimeout'),
Expand All @@ -239,5 +239,13 @@ export const advancedSettingsFields = [
];

export const emptyAdvancedSettings = advancedSettingsFields.reduce((obj, advancedSetting) => {
return { ...obj, [advancedSetting.field]: '' };
const { field, defaultValue } = advancedSetting;
return { ...obj, [field]: defaultValue };
}, {});

export function areAdvancedSettingsEdited(followerIndex) {
return advancedSettingsFields.some(advancedSetting => {
const { field } = advancedSetting;
return followerIndex[field] !== emptyAdvancedSettings[field];
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be simplified as

export const areAdvancedSettingsEdited = (followerIndex) => (
  advancedSettingsFields.some(({ field }) => followerIndex[field] !== emptyAdvancedSettings[field])
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your suggestion is terser, but I find the extra variable name advancedSetting helps me read the code like a story instead of having to mentally map ({ field }) to an entity. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Welcome to functional programming 😄 Not any more complex than any other deconstruct IMO

Copy link
Contributor Author

@cjcenizal cjcenizal Jan 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, I'm familiar with functional programming paradigms. I just don't always apply them if I think they degrade readability.

125 changes: 59 additions & 66 deletions ...ross_cluster_replication/public/app/components/follower_index_form/follower_index_form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import {
EuiButton,
EuiButtonEmpty,
EuiCallOut,
EuiCode,
EuiDescribedFormGroup,
EuiFlexGroup,
EuiFlexItem,
Expand All @@ -26,6 +25,7 @@ import {
EuiLoadingSpinner,
EuiOverlayMask,
EuiSpacer,
EuiSwitch,
EuiText,
EuiTitle,
} from '@elastic/eui';
Expand All @@ -36,7 +36,11 @@ import { loadIndices } from '../../services/api';
import { API_STATUS } from '../../constants';
import { SectionError } from '../section_error';
import { FormEntryRow } from '../form_entry_row';
import { advancedSettingsFields, emptyAdvancedSettings } from './advanced_settings_fields';
import {
advancedSettingsFields,
emptyAdvancedSettings,
areAdvancedSettingsEdited,
} from './advanced_settings_fields';
import { extractQueryParams } from '../../services/query_params';
import { getRemoteClusterName } from '../../services/get_remote_cluster_name';
import { RemoteClustersFormField } from '../remote_clusters_form_field';
Expand Down Expand Up @@ -93,16 +97,20 @@ export const FollowerIndexForm = injectI18n(
constructor(props) {
super(props);

const isNew = this.props.followerIndex === undefined;
const { route: { location: { search } } } = routing.reactRouter;
const queryParams = extractQueryParams(search);

const isNew = this.props.followerIndex === undefined;
const remoteClusterName = getRemoteClusterName(this.props.remoteClusters, queryParams.cluster);
const followerIndex = isNew
? getEmptyFollowerIndex(remoteClusterName)
: {
...getEmptyFollowerIndex(),
...this.props.followerIndex,
};
const areAdvancedSettingsVisible = isNew ? false : ( // eslint-disable-line no-nested-ternary
areAdvancedSettingsEdited(followerIndex) ? true : false
);

const fieldsErrors = this.getFieldsErrors(followerIndex);

Expand All @@ -111,10 +119,11 @@ export const FollowerIndexForm = injectI18n(
followerIndex,
fieldsErrors,
areErrorsVisible: false,
areAdvancedSettingsVisible: isNew ? false : true,
areAdvancedSettingsVisible,
isValidatingIndexName: false,
};

this.cachedAdvancedSettings = {};
this.validateIndexName = debounce(this.validateIndexName, 500);
}

Expand Down Expand Up @@ -212,34 +221,39 @@ export const FollowerIndexForm = injectI18n(
return this.state.followerIndex;
};

toggleAdvancedSettings = () => {
this.setState(({ areAdvancedSettingsVisible, cachedAdvancedSettings }) => {
// Hide settings, clear fields, and create cache.
if (areAdvancedSettingsVisible) {
const fields = this.getFields();
toggleAdvancedSettings = (event) => {
// If the user edits the advanced settings but then hides them, we need to make sure the
// edited values don't get sent to the API when the user saves, but we *do* want to restore
// these values to the form when the user re-opens the advanced settings.
if (event.target.checked) {
// Apply the cached advanced settings to the advanced settings form.
this.onFieldsChange(this.cachedAdvancedSettings);

const newCachedAdvancedSettings = advancedSettingsFields.reduce((cache, { field }) => {
const value = fields[field];
if (value !== '') {
cache[field] = value;
}
return cache;
}, {});
// Reset the cache of the advanced settings.
this.cachedAdvancedSettings = {};

this.onFieldsChange(emptyAdvancedSettings);
// Show the advanced settings.
return this.setState({
areAdvancedSettingsVisible: true,
});
}

return {
areAdvancedSettingsVisible: false,
cachedAdvancedSettings: newCachedAdvancedSettings,
};
// Clear the advanced settings form.
this.onFieldsChange(emptyAdvancedSettings);

// Save a cache of the advanced settings.
const fields = this.getFields();
this.cachedAdvancedSettings = advancedSettingsFields.reduce((cache, { field }) => {
const value = fields[field];
if (value !== '') {
cache[field] = value;
}
return cache;
}, {});

// Show settings and restore fields from the cache.
this.onFieldsChange(cachedAdvancedSettings);
return {
areAdvancedSettingsVisible: true,
cachedAdvancedSettings: {},
};
// Hide the advanced settings.
this.setState({
areAdvancedSettingsVisible: false,
});
}

Expand Down Expand Up @@ -455,22 +469,7 @@ export const FollowerIndexForm = injectI18n(
* Advanced settings
*/

const toggleAdvancedSettingButtonLabel = areAdvancedSettingsVisible
? (
<FormattedMessage
id="xpack.crossClusterReplication.followerIndex.advancedSettingsForm.hideButtonLabel"
defaultMessage="Don't use advanced settings"
/>
) : (
<FormattedMessage
id="xpack.crossClusterReplication.followerIndex.advancedSettingsForm.showButtonLabel"
defaultMessage="Use advanced settings"
/>
);

const renderAdvancedSettings = () => {
const { isNew } = this.state;

return (
<Fragment>
<EuiHorizontalRule />
Expand All @@ -480,7 +479,7 @@ export const FollowerIndexForm = injectI18n(
<h2>
<FormattedMessage
id="xpack.crossClusterReplication.followerIndexForm.advancedSettingsTitle"
defaultMessage="Advanced settings"
defaultMessage="Advanced settings (optional)"
/>
</h2>
</EuiTitle>
Expand All @@ -490,17 +489,21 @@ export const FollowerIndexForm = injectI18n(
<p>
<FormattedMessage
id="xpack.crossClusterReplication.followerIndexForm.advancedSettingsDescription"
defaultMessage="Use advanced settings to control the rate at which data is replicated."
defaultMessage="Customize advanced settings to control the rate at which data is replicated.
If you don't customize them, default advanced settings will be applied."
/>
</p>
{isNew ? (
<EuiButton
color="primary"
onClick={this.toggleAdvancedSettings}
>
{toggleAdvancedSettingButtonLabel}
</EuiButton>
) : null}

<EuiSwitch
label={(
<FormattedMessage
id="xpack.crossClusterReplication.followerIndex.advancedSettingsForm.showSwitchLabel"
defaultMessage="Customize advanced settings"
/>
)}
checked={areAdvancedSettingsVisible}
onChange={this.toggleAdvancedSettings}
/>
</Fragment>
)}
fullWidth
Expand All @@ -512,33 +515,23 @@ export const FollowerIndexForm = injectI18n(
<Fragment>
<EuiSpacer size="s"/>
{advancedSettingsFields.map((advancedSetting) => {
const { field, title, description, label, helpText, defaultValue } = advancedSetting;
const { field, title, description, label, helpText, defaultValue, type } = advancedSetting;
return (
<FormEntryRow
key={field}
field={field}
value={followerIndex[field]}
defaultValue={defaultValue}
error={fieldsErrors[field]}
title={(
<EuiTitle size="xs">
<h3>{title}</h3>
</EuiTitle>
)}
description={(
<Fragment>
{description}
<EuiSpacer size="s" />
<EuiText size="xs">
<FormattedMessage
id="xpack.crossClusterReplication.followerIndexForm.advancedSettingsDefaultLabel"
defaultMessage="Default:"
/>{' '}
<EuiCode>{defaultValue}</EuiCode>
</EuiText>
</Fragment>
)}
description={description}
label={label}
helpText={helpText}
type={type}
areErrorsVisible={areErrorsVisible}
onValueUpdate={this.onFieldsChange}
/>
Expand Down
Loading