[App Search] Added the log retention confirmation modal to the Settings page by JasonStoltz · Pull Request #83009 · elastic/kibana

JasonStoltz · 2020-11-09T19:55:39Z

Summary

This PR adds the confirmation modal for log retention settings to the App Search Settings page.

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)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

JasonStoltz · 2020-11-10T14:08:09Z

~~This PR is in draft until #82982 is merged. But it's slow going getting that merged, I had to update it to resolve conflicts so now I have to wait for CI again.~~

~~You can get a head start on reviewing this PR if you'd like. Just focus on this commit: 680b28e~~

UPDATE: This is ready for review

...tions/app_search/components/settings/log_retention/log_retention_confirmation_modal.test.tsx

cee-chen

Awesome stuff! QA and mobile/cross-browser testing looks great. Some minor test and i18n/a11y comments but overall looks great.

...lic/applications/app_search/components/settings/log_retention/generic_confirmation_modal.tsx

cee-chen · 2020-11-10T22:57:17Z

...plications/app_search/components/settings/log_retention/log_retention_confirmation_modal.tsx

+      )}
+      {openedModal === ELogRetentionOptions.API && (


Just me thinking out loud: I wonder what the impetus was for creating multiple modals instead of a single modal where the content gets changed depending on the option selected - I personally would probably have opted for the latter 🤔

Yeah that may have been cleaner.

...plications/app_search/components/settings/log_retention/log_retention_confirmation_modal.tsx

cee-chen · 2020-11-10T23:05:25Z

...tions/app_search/components/settings/log_retention/log_retention_confirmation_modal.test.tsx

+    // Remove the jest mock on createPortal
+    // @ts-ignore
+    ReactDOM.createPortal.mockClear();


why is this ts-ignore and comment here? Do we need it, or can we do

Suggested change

// Remove the jest mock on createPortal

// @ts-ignore

ReactDOM.createPortal.mockClear();

(ReactDOM.createPortal as jest.Mock).mockClear();

or just jest.clearAllMocks 🤷
edit: which we're already doing in beforeEach - can we lose this block entirely?

Deleted per previous comment.

...pplications/app_search/components/settings/log_retention/generic_confirmation_modal.test.tsx

cee-chen · 2020-11-10T23:14:28Z

...lic/applications/app_search/components/settings/log_retention/generic_confirmation_modal.tsx

+          {i18n.translate('xpack.enterpriseSearch.appSearch.settings.logRetention.modal.cancel', {
+            defaultMessage: 'Cancel',


[not a change request, just me thinking out loud]
I wonder if we should have a library of common UI strings like 'Cancel', 'Save', 'Edit', 'Delete' etc., to lower the number of i18n strings we have and the number of translations the team has to do

Happy to do this is a tech debt refactor in a separate PR later if you think it's a good idea 🤷

Yeah that's a good point!

...tions/app_search/components/settings/log_retention/log_retention_confirmation_modal.test.tsx

...lic/applications/app_search/components/settings/log_retention/generic_confirmation_modal.tsx

cee-chen · 2020-11-10T23:25:56Z

...lic/applications/app_search/components/settings/log_retention/generic_confirmation_modal.tsx

+        >
+          <EuiFieldText
+            data-test-subj="GenericConfirmationModalInput"
+            name="engineName"


this name doesn't seem right? do we even need a name prop

Suggested change

name="engineName"

...lic/applications/app_search/components/settings/log_retention/generic_confirmation_modal.tsx

JasonStoltz · 2020-11-11T14:55:37Z

@constancecchen Ready for a second look

cee-chen

This looks great - thank you so much for the changes! Really excited by how much we're improving our codebase for international users and users with disabilities.

kibanamachine · 2020-11-11T16:38:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 42ebfdc

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	461	463	+2

Async chunks

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

id	before	after	diff
`enterpriseSearch`	682.7KB	691.6KB	+8.9KB

History

💚 Build #87115 succeeded 1f3cd7d
💚 Build #86916 succeeded 680b28e

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

…gs page (elastic#83009)

…gs page (#83009) (#83191)

…ts-public * upstream/master: (57 commits) Remove unused asciidoc file (elastic#83228) [Lens] Remove background from lens embeddable (elastic#83061) [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991) Removing unnecessary trailing slash in CODEOWNERS Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236) Trying to fix CODEOWERS, missing a starting slash (elastic#83233) skip flaky suite (elastic#83231) Add enzyme rerender test helper (elastic#83208) Move Elasticsearch type definitions out of APM (elastic#83081) [ts/checkTsProjects] produce a more useful error message (elastic#83209) [kbnClient] retry updating config if necessary (elastic#83205) I accidentally removed this line in a recent PR (elastic#83201) Don't make the caller do work the function can do (elastic#83180) [App Search] Update EngineRouter & EngineNav to use EngineLogic (elastic#83138) [Workplace Search] Add routes for Sources (elastic#83125) Update logstash pipeline management to use system index APIs (elastic#80405) [ML] Replace EuiBasicTable with EuiInMemoryTable (elastic#83057) [Metrics UI] Add basic interaction and shell for node details overlay (elastic#82013) [App Search] Added the log retention confirmation modal to the Settings page (elastic#83009) [docs] Fix create map title in import geospatial page (elastic#83172) ...

JasonStoltz added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 9, 2020

JasonStoltz mentioned this pull request Nov 9, 2020

[App Search] Added the log retention panel to the Settings page #82982

Merged

7 tasks

JasonStoltz requested a review from cee-chen November 10, 2020 14:08

JasonStoltz force-pushed the log-retention-ui-2 branch from 680b28e to 036cfb1 Compare November 10, 2020 15:30

Added log retention modal

1f3cd7d

JasonStoltz force-pushed the log-retention-ui-2 branch from 036cfb1 to 1f3cd7d Compare November 10, 2020 15:34

JasonStoltz marked this pull request as ready for review November 10, 2020 15:35

JasonStoltz requested a review from a team November 10, 2020 15:35

cee-chen reviewed Nov 10, 2020

View reviewed changes

...tions/app_search/components/settings/log_retention/log_retention_confirmation_modal.test.tsx Outdated Show resolved Hide resolved

cee-chen reviewed Nov 10, 2020

View reviewed changes

JasonStoltz added 7 commits November 11, 2020 08:50

No portal

99463bb

i18n DISABLE text

ee43a03

Better red disable text

2737c20

Better a11y

e847930

engineName?

5df1e78

case insensitive disable

e220982

Merge branch 'master' into log-retention-ui-2

42ebfdc

JasonStoltz requested a review from cee-chen November 11, 2020 14:55

cee-chen approved these changes Nov 11, 2020

View reviewed changes

JasonStoltz merged commit 83b52bf into elastic:master Nov 11, 2020

JasonStoltz deleted the log-retention-ui-2 branch November 11, 2020 16:46

JasonStoltz mentioned this pull request Nov 11, 2020

[7.x] [App Search] Added the log retention confirmation modal to the Settings page (#83009) #83191

Merged

JasonStoltz added a commit to JasonStoltz/kibana that referenced this pull request Nov 11, 2020

[App Search] Added the log retention confirmation modal to the Settin…

3992066

…gs page (elastic#83009)

JasonStoltz added a commit that referenced this pull request Nov 11, 2020

[App Search] Added the log retention confirmation modal to the Settin…

198184e

…gs page (#83009) (#83191)

		{i18n.translate('xpack.enterpriseSearch.appSearch.settings.logRetention.modal.cancel', {
		defaultMessage: 'Cancel',

Conversation

JasonStoltz commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

JasonStoltz commented Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JasonStoltz commented Nov 11, 2020

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Nov 11, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JasonStoltz commented Nov 9, 2020 •

edited

Loading

JasonStoltz commented Nov 10, 2020 •

edited

Loading