[App Search] Add header details to the Result Settings page #96623

JasonStoltz · 2021-04-08T17:58:51Z

Summary

This PR adds a header description and actions buttons. It also removes custom modal logic and replaces that with standard window.confirm messages.

These buttons will eventually have additional states, there are certain conditions where they may not appear or be disabled. I will add that in my next PR.

The commits in this PR are atomic, I recommend following along commit by commit.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

I assume so since it's just EUI:

This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Also changed it so that it would read the current settings from state, rather than requiring them to be read and passed back in as it was in ent-search

JasonStoltz · 2021-04-08T18:00:36Z

...ch/public/applications/app_search/components/relevance_tuning/relevance_tuning_logic.test.ts

@@ -586,10 +586,9 @@ describe('RelevanceTuningLogic', () => {
        confirmSpy.mockImplementation(() => false);

        RelevanceTuningLogic.actions.resetSearchSettings();
+        await nextTick();


This was unrelated to the current changes, but I added since I was using this test as a reference.

Without this, it would return a false positive always.

++ for the expect cleanup below too!

JasonStoltz · 2021-04-08T18:03:18Z

...se_search/public/applications/app_search/components/result_settings/result_settings_logic.ts

+        try {
+          response = await http.put(url, {
+            body: JSON.stringify({
+              result_fields: values.reducedServerResultFields,


This previous required reducedServerResultFields to be passed in from the consuming component, which seemed unnecessary, so I changed it to grab the value directly from state.

cee-chen · 2021-04-09T14:54:27Z

...se_search/public/applications/app_search/components/result_settings/result_settings.test.tsx

@@ -32,8 +37,10 @@ describe('RelevanceTuning', () => {
    jest.clearAllMocks();
  });

+  const subject = () => shallow(<ResultSettings engineBreadcrumb={['test']} />);


I like this! I think I might start stealing this more. But I also like rerender depending on the test/component 🤔 Haha, I'm probably overthinking it

Ah, OK. I hadn't thought of using rerender for this. This is primarily just to DRY up the code a little bit. The subject pattern is something we use in Rails tests in ent-search.

cee-chen · 2021-04-09T14:55:42Z

...se_search/public/applications/app_search/components/result_settings/result_settings.test.tsx

    expect(wrapper.find(ResultSettingsTable).exists()).toBe(false);
    expect(wrapper.find(SampleResponse).exists()).toBe(false);
  });
+
+  it('renders a "save" button that will save the current changes', () => {
+    const buttons = subject().find(EuiPageHeader).prop('rightSideItems') as React.ReactElement[];


[optional] maybe a candidate for another test helper since it's repeated a few times? e.g.

const buttonsSubject = () => subject().find(EuiPageHeader).prop('rightSideItems') as React.ReactElement[];

3e51755

I updated tests in Relevance Tuning as well since they were in the same situation.

I took a slightly different approach... I created a selector function (findButtons) which accepts a ShallowWrapper (i.e., subject()) rather than one that calls subject() directly.

It's irrelevant for this instance but as a point of pattern the composability is handy if you have a situation where you need to create the subject differently depending on the test.

cee-chen

One super minor change request, this otherwise looks great! Thanks for the super atomic commits & PRs Jason 🙇‍♀️

cee-chen · 2021-04-09T14:59:38Z

...erprise_search/public/applications/app_search/components/result_settings/result_settings.tsx

+const RESTORE_DEFAULTS_BUTTON_LABEL = i18n.translate(
+  'xpack.enterpriseSearch.appSearch.engine.resultSettings.restoreDefaultsButtonLabel',
+  { defaultMessage: 'Restore defaults' }
+);
+
+const SAVE_BUTTON_LABEL = i18n.translate(
+  'xpack.enterpriseSearch.appSearch.engine.resultSettings.saveButtonLabel',
+  { defaultMessage: 'Save' }
+);
+


[Change request] I DRY'd out these i18n labels a while back - you can delete these and directly import from shared/constants/actions.ts and app_search/constants.ts

import { SAVE_BUTTON_LABEL } from '../../../shared/constants'; import { RESTORE_DEFAULTS_BUTTON_LABEL } from '../../constants';

Whoops, that's so handy, can't believe I forgot about these 🤦 : 9971bfe

JasonStoltz · 2021-04-12T15:43:14Z

@constancecchen Bonehead move on my part, I realized just now that I didn't create a different message for the "Reset" button 🤦 : 5455a45

cee-chen

Changes LGTM! 👍

kibanamachine · 2021-04-12T17:38:42Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1323	1322	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`enterpriseSearch`	2.0MB	2.0MB	+1.3KB

History

💚 Build #118352 succeeded d055692

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…96623)

kibanamachine · 2021-04-12T17:41:36Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…96853) Co-authored-by: Jason Stoltzfus <[email protected]>

JasonStoltz added 5 commits April 8, 2021 10:49

Add a description to the page

0d1722d

Per i18n guide, this should have punctuation

2673009

Remove all custom confirmation modals

d657797

Convert saveResultSettings to use window.confirm

93d35aa

Also changed it so that it would read the current settings from state, rather than requiring them to be read and passed back in as it was in ent-search

Convert resetAllFields to use window.confirm

32771ed

JasonStoltz added release_note:skip Skip the PR/issue when compiling release notes v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Apr 8, 2021

JasonStoltz requested a review from a team April 8, 2021 17:58

JasonStoltz commented Apr 8, 2021

View reviewed changes

Added header action buttons

d055692

JasonStoltz force-pushed the chrome branch from 24b12ad to d055692 Compare April 8, 2021 18:01

JasonStoltz commented Apr 8, 2021

View reviewed changes

cee-chen reviewed Apr 9, 2021

View reviewed changes

cee-chen suggested changes Apr 9, 2021

View reviewed changes

JasonStoltz added 4 commits April 12, 2021 11:29

DRYer test helpers

3e51755

Use shared constants for button labels

9971bfe

Using the wrong confirmation message

5455a45

Merge branch 'master' into chrome

e39c357

JasonStoltz requested a review from cee-chen April 12, 2021 15:43

cee-chen approved these changes Apr 12, 2021

View reviewed changes

JasonStoltz enabled auto-merge (squash) April 12, 2021 16:29

JasonStoltz merged commit b4d3302 into elastic:master Apr 12, 2021

kibanamachine mentioned this pull request Apr 12, 2021

[7.x] [App Search] Add header details to the Result Settings page (#96623) #96853

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 12, 2021

[App Search] Add header details to the Result Settings page (elastic#…

f8b2418

…96623)

kibanamachine added a commit that referenced this pull request Apr 12, 2021

[App Search] Add header details to the Result Settings page (#96623) (#…

b09b3d7

…96853) Co-authored-by: Jason Stoltzfus <[email protected]>

JasonStoltz deleted the chrome branch April 22, 2021 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Add header details to the Result Settings page #96623

[App Search] Add header details to the Result Settings page #96623

JasonStoltz commented Apr 8, 2021 •

edited

Loading

JasonStoltz Apr 8, 2021

cee-chen Apr 9, 2021

JasonStoltz Apr 8, 2021

cee-chen Apr 9, 2021

JasonStoltz Apr 12, 2021

cee-chen Apr 9, 2021 •

edited

Loading

JasonStoltz Apr 12, 2021

cee-chen left a comment

cee-chen Apr 9, 2021

JasonStoltz Apr 12, 2021

JasonStoltz commented Apr 12, 2021

cee-chen left a comment

kibanamachine commented Apr 12, 2021

kibanamachine commented Apr 12, 2021

[App Search] Add header details to the Result Settings page #96623

[App Search] Add header details to the Result Settings page #96623

Conversation

JasonStoltz commented Apr 8, 2021 • edited Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz commented Apr 12, 2021

cee-chen left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 12, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Apr 12, 2021

💚 Backport successful

JasonStoltz commented Apr 8, 2021 •

edited

Loading

cee-chen Apr 9, 2021 •

edited

Loading