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

Fix minor Console regressions introduced during EUIfication. #41089

Merged
merged 5 commits into from
Jul 25, 2019
Merged

Fix minor Console regressions introduced during EUIfication. #41089

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d7db0b9
Fix minor Console regressions introduced during EUIfication.
cjcenizal Jul 14, 2019
8bc9011
Merge branch 'master' into console/eui-regressions
cjcenizal Jul 24, 2019
5d575ed
Fix TS error.
cjcenizal Jul 24, 2019
871cbd2
Merge branch 'master' into console/eui-regressions
cjcenizal Jul 25, 2019
f8f0372
Remove redundant condition.
cjcenizal Jul 25, 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
4 changes: 4 additions & 0 deletions src/legacy/core_plugins/console/public/_app.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,7 @@
z-index: $euiZLevel1 + 2;
margin-top: 22px;
}

.conApp__settingsModal {
min-width: 460px;
}
2 changes: 1 addition & 1 deletion src/legacy/core_plugins/console/public/src/components/editor_example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import React, { useEffect } from 'react';
// @ts-ignore
import exampleText from 'raw-loader!./helpExample.txt';
import exampleText from 'raw-loader!./help_example.txt';
import $ from 'jquery';
// @ts-ignore
import SenseEditor from '../sense_editor/editor';
Expand Down
0 ...ole/public/src/components/helpExample.txt → ...le/public/src/components/help_example.txt
View file
Open in desktop
File renamed without changes.
106 changes: 66 additions & 40 deletions src/legacy/core_plugins/console/public/src/components/settings_modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import React, { useState } from 'react';
import React, { Fragment, useState } from 'react';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';

Expand Down Expand Up @@ -106,9 +106,70 @@ export function DevToolsSettingsModal(props: Props) {
});
}

// It only makes sense to show polling options if the user needs to fetch any data.
const pollingFields =
fields || indices || templates ? (
<Fragment>
<EuiFormRow
label={
<FormattedMessage
id="console.settingsPage.refreshingDataLabel"
defaultMessage="Refreshing autocomplete suggestions"
/>
}
helpText={
<FormattedMessage
id="console.settingsPage.refreshingDataDescription"
defaultMessage="Console refreshes autocomplete suggestions by querying Elasticsearch.
Automatic refreshes may be an issue if you have a large cluster or if you have network limitations."
/>
}
>
<EuiSwitch
checked={polling}
data-test-subj="autocompletePolling"
id="autocompletePolling"
label={
<FormattedMessage
defaultMessage="Automatically refresh autocomplete suggestions"
id="console.settingsPage.pollingLabelText"
/>
}
onChange={e => setPolling(e.target.checked)}
/>
</EuiFormRow>

<EuiButton
data-test-subj="autocompletePolling"
id="autocompletePolling"
onClick={() => {
// Only refresh the currently selected settings.
props.refreshAutocompleteSettings({
autocomplete: {
fields,
indices,
templates,
},
});
}}
>
<FormattedMessage
defaultMessage="Refresh autocomplete suggestions"
id="console.settingsPage.refreshButtonLabel"
/>
</EuiButton>
</Fragment>
) : (
undefined
);

return (
<EuiOverlayMask>
<EuiModal data-test-subj="devToolsSettingsModal" onClose={props.onClose}>
<EuiModal
data-test-subj="devToolsSettingsModal"
className="conApp__settingsModal"
onClose={props.onClose}
>
<EuiModalHeader>
<EuiModalHeaderTitle>
<FormattedMessage
Expand Down Expand Up @@ -140,6 +201,7 @@ export function DevToolsSettingsModal(props: Props) {
}}
/>
</EuiFormRow>

<EuiFormRow>
<EuiSwitch
checked={wrapMode}
Expand Down Expand Up @@ -176,6 +238,7 @@ export function DevToolsSettingsModal(props: Props) {
onChange={e => setTripleQuotes(e.target.checked)}
/>
</EuiFormRow>

<EuiFormRow
labelType="legend"
label={
Expand All @@ -193,45 +256,8 @@ export function DevToolsSettingsModal(props: Props) {
}}
/>
</EuiFormRow>
<EuiFormRow
label={
<FormattedMessage
id="console.settingsPage.refreshingDataLabel"
defaultMessage="Refreshing autocomplete suggestions"
/>
}
helpText={
<FormattedMessage
id="console.settingsPage.refreshingDataDescription"
defaultMessage="Console refreshes autocomplete suggestions by querying Elasticsearch.
Automatic refreshes may be an issue if you have a large cluster or if you have network limitations."
/>
}
>
<EuiSwitch
checked={polling}
data-test-subj="autocompletePolling"
id="autocompletePolling"
label={
<FormattedMessage
defaultMessage="Automatically refresh autocomplete suggestions"
id="console.settingsPage.pollingLabelText"
/>
}
onChange={e => setPolling(e.target.checked)}
/>
</EuiFormRow>

<EuiButton
data-test-subj="autocompletePolling"
id="autocompletePolling"
onClick={props.refreshAutocompleteSettings}
>
<FormattedMessage
defaultMessage="Refresh autocomplete suggestions"
id="console.settingsPage.refreshButtonLabel"
/>
</EuiButton>
{pollingFields}
</EuiModalBody>

<EuiModalFooter>
Expand Down
22 changes: 16 additions & 6 deletions src/legacy/core_plugins/console/public/src/helpers/settings_show_modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ export function showSettingsModal() {
const container = document.getElementById('consoleSettingsModal');
const curSettings = getCurrentSettings();

const refreshAutocompleteSettings = () => {
mappings.retrieveAutoCompleteInfo();
const refreshAutocompleteSettings = selectedSettings => {
mappings.retrieveAutoCompleteInfo(selectedSettings);
};

const closeModal = () => {
Expand All @@ -54,12 +54,22 @@ export function showSettingsModal() {
prevSettings: DevToolsSettings
) => {
// We'll only retrieve settings if polling is on.
const isPollingChanged = prevSettings.polling !== newSettings.polling;
if (newSettings.polling) {
const autocompleteDiff = getAutocompleteDiff(newSettings, prevSettings);
if (autocompleteDiff.length > 0) {
mappings.retrieveAutoCompleteInfo(newSettings.autocomplete);
} else if (isPollingChanged) {

const isSettingsChanged = autocompleteDiff.length > 0;
const isPollingChanged = prevSettings.polling !== newSettings.polling;

if (isSettingsChanged) {
// If the user has changed one of the autocomplete settings, then we'll fetch just the
// ones which have changed.
const changedSettings = autocompleteDiff.reduce((changedSettingsAccum, setting) => {
changedSettingsAccum[setting] = newSettings.autocomplete[setting];
return changedSettingsAccum;
}, {});
mappings.retrieveAutoCompleteInfo(changedSettings);
} else if (isPollingChanged && newSettings.polling) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is && newSettings.polling needed? Looks like it is already being checked on line 57.

After looking at this further, should the initial check on line 57 be removed? I think we would want to fetch any changed settings regardless if polling is turned on or off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My line of thought here is that users disabled polling because the fetch request is so expensive. So, I think they'd be unhappy with any fetch that's automatically executed without them clicking the "Fetch now" button. Does that make sense? I'm going to add a comment to explain that for now but I'm still open to changing this if you think we can improve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense. Thanks for adding the comment! I think you can still delete && newSettings.polling (my original comment). Otherwise, this is fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isPollingChanged will also be true if the user has turned off polling. So we need to make sure we're fetching the settings only if the user has turned polling on, but not when they've turned it off.

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't that already being handled on line 60 though?

if (newSettings.polling) { ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you're right! I missed that. Thank you for persisting in pointing it out to me 😅 Will fix!

// If the user has turned polling on, then we'll fetch all selected autocomplete settings.
mappings.retrieveAutoCompleteInfo();
}
}
Expand Down
10 changes: 10 additions & 0 deletions src/legacy/core_plugins/console/public/src/mappings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,16 @@ function retrieveSettings(settingsKey, settingsToRetrieve) {
}

// Retrieve all selected settings by default.
// TODO: We should refactor this to be easier to consume. Ideally this function should retrieve
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this!

// whatever settings are specified, otherwise just use the saved settings. This requires changing
// the behavior to not *clear* whatever settings have been unselected, but it's hard to tell if
// this is possible without altering the autocomplete behavior. These are the scenarios we need to
// support:
// 1. Manual refresh. Specify what we want. Fetch specified, leave unspecified alone.
// 2. Changed selection and saved: Specify what we want. Fetch changed and selected, leave
// unchanged alone (both selected and unselected).
// 3. Poll: Use saved. Fetch selected. Ignore unselected.

function retrieveAutoCompleteInfo(settingsToRetrieve = settings.getAutocomplete()) {
if (pollTimeoutId) {
clearTimeout(pollTimeoutId);
Expand Down