Do not perform unnecessary round-trip to ES.#7960
Do not perform unnecessary round-trip to ES.#7960thomasneirynck merged 3 commits intoelastic:masterfrom
Conversation
|
jenkins, test this |
There was a problem hiding this comment.
this function is invoked with the angular-binding in sidebar.html (cf. stageEditableVis)
|
jenkins test this |
There was a problem hiding this comment.
Small suggestion. I think we make this a little more readable by nesting conditions, exiting early, computing values near where they're needed, and putting comments on their own line:
if (stage) {
// Only requery ES when the query has changed. vislib params are irrelevant.
const fromVisLib = getVisLibOptions(fromVis);
const toVisLib = getVisLibOptions(toVis);
if (_.isEqual(toVisLib, fromVisLib) {
return $scope.fetch();
}
}
$state.save();I'm also not sure what that comment means in the context of that condition... can you explain some more?
|
I'm not totally sure, but we had an issue with master earlier, so this build may pass if you rebase. |
If the visualization parameters change, but the query stays intact, we should not fetch a new query.
64dfe4b to
0b13617
Compare
|
jenkins, test this |
|
failing screenshot test after rebasing. Need to investigate ... |
0b13617 to
8e8721c
Compare
The below is a more clear way to express what needs to happen. This also avoids a bug when there is no re-fetch when there are both changes in the Options and Data tab.
8e8721c to
433c842
Compare
|
@cjcenizal can you check once more? I made a change to the original you reviewed. The play-button triggers a visualization when there is a change in the "Data"-tab. (Instead of 'no changes in the "Options" tab'). |
| * Ignores the non-array indexes | ||
| * @param aggConfigs an AggConfigs instance | ||
| */ | ||
| AggConfigs.prototype.jsonDataEquals = function (aggConfigs) { |
|
Functionally this LGTM, some comments above about testing. |
There was a problem hiding this comment.
This might be clearer if we prefix with is, has, or should to indicate the boolean nature of the var. How about isAggregationsChanged?
|
Nice work! Just had a couple readability-related suggestions. |
Also test .toJSON orders properties consistently.
4fb8601 to
ca6ce25
Compare
|
Ahhh, great names! So much more context! Thank you. LGTM! |
|
jenkins, test this |
|
LGTM |
Do not perform unnecessary round-trip to ES. Former-commit-id: 0c9e2af
`v95.7.0` ⏩ `v95.9.0` > [!CAUTION] > This PR contains the final set of Emotion conversions for all EuiForm components. > If your plugin contains any custom CSS/styling to **EuiFormRow, EuiFormControlLayout, EuiCheckbox, EuiRadio, or EuiSwitch**,⚠️ make sure to QA your UI to ensure no visual regressions have occurred!⚠️ --- ## [`v95.9.0`](https://github.com/elastic/eui/releases/v95.9.0) - Updated `EuiSearchBar`'s optional `box.schema` prop with a new `recognizedFields` configuration. This allows specifying the phrases that will be parsed as field clauses ([#7960](elastic/eui#7960)) - Updated `EuiIcon` with a new `tokenSemanticText` glyph ([#7971](elastic/eui#7971)) - Added support for TypeScript 5 ([#7980](elastic/eui#7980)) **Bug fixes** - Fixed `EuiSelectableTemplateSitewide` styles when used within a dark-themed `EuiHeader` ([#7977](elastic/eui#7977)) ## [`v95.8.0`](https://github.com/elastic/eui/releases/v95.8.0) - Updated `EuiHeaderLinks`'s mobile menu to set a slight popover padding by default ([#7961](elastic/eui#7961)) - This can be overridden via `popoverProps.panelPaddingSize` if needed. - Updated `EuiHeaderLink` to default to a size of `s` (down from `m`) ([#7961](elastic/eui#7961)) **Accessibility** - Updated the `aria-label` attribute for the `EuiFieldSearch` clear button ([#7970](elastic/eui#7970)) **Bug fixes** - Fixed a visual bug with `<EuiDualRange showInput="inputWithPopover" />` form controls ([#7957](elastic/eui#7957)) **Deprecations** - Deprecated `EuiFormRow`'s `columnCompressedSwitch` display prop. Use `columnCompressed` instead, which will automatically account for child `EuiSwitch`es ([#7968](elastic/eui#7968)) - Deprecated `EuiFormRow`'s `rowCompressed` display prop. Use `row` instead for vertical forms, or `centerCompressed` for inline forms ([#7968](elastic/eui#7968)) - (Styling) Updated `EuiFormRow`'s `hasEmptySpaceLabel` prop to no longer attempt to automatically align its content to a vertical center. Use the `display="center"` prop for that instead ([#7968](elastic/eui#7968)) **CSS-in-JS conversions** - Converted `EuiFormControlLayout` to Emotion ([#7954](elastic/eui#7954)) - Removed `.euiFormControlLayout--*icons` classNames and `--eui-form-control-layout-icons-padding` CSS var. Use `--euiFormControlRightIconsCount` or `--euiFormControlLeftIconsCount` instead - Converted `EuiFormLayoutDelimited` to Emotion ([#7957](elastic/eui#7957)) - Fixed `cloneElementWithCss` throwing an error when used multiple times without a `key` prop ([#7957](elastic/eui#7957)) - Updated `cloneElementWithCss` utility to support a third argument that allows prepending vs. appending the cloned Emotion css className ([#7957](elastic/eui#7957)) - Removed `@euiFormControlLayoutClearIcon` Sass mixin ([#7959](elastic/eui#7959)) - Converted `EuiDescribedFormGroup` to Emotion ([#7964](elastic/eui#7964)) - Converted `EuiForm`, `EuiFormHelpText`, and `EuiFormErrorText` to Emotion ([#7966](elastic/eui#7966)) - Converted `EuiFormLabel` and `EuiFormLegend` to Emotion; Removed `@euiFormLabel` mixin ([#7967](elastic/eui#7967)) - Converted `EuiFormRow` to Emotion ([#7968](elastic/eui#7968)) - Converted `EuiCheckbox` to Emotion ([#7969](elastic/eui#7969)) - Converted `EuiRadio` to Emotion ([#7969](elastic/eui#7969)) - Converted `EuiSwitch` to Emotion ([#7969](elastic/eui#7969)) - Removed the following Sass variables: ([#7969](elastic/eui#7969)) - `$euiFormCustomControlDisabledIconColor` - `$euiFormCustomControlBorderColor` - `$euiRadioSize` - `$euiCheckBoxSize` - `$euiCheckboxBorderRadius` - `$euiSwitchHeight` (and compressed/mini variants) - `$euiSwitchWidth` (and compressed/mini variants) - `$euiSwitchThumbSize` (and compressed/mini variants) - `$euiSwitchIconHeight` - `$euiSwitchOffColor` - Removed the following Sass mixins: ([#7969](elastic/eui#7969)) - `euiIconBackground` - `euiCustomControl` - `euiCustomControlSelected` - `euiCustomControlDisabled` - `euiCustomControlFocused` --------- Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>
In place fix for #7945.
Skips ES fetch when there are no changes in the request-parameters after pressing the "play" button in the side-bar.