Skip to content

[management] Adjustments to Settings components and utilities#166413

Merged
clintandrewhall merged 3 commits intoelastic:mainfrom
clintandrewhall:advanced_settings/field_updates
Sep 19, 2023
Merged

[management] Adjustments to Settings components and utilities#166413
clintandrewhall merged 3 commits intoelastic:mainfrom
clintandrewhall:advanced_settings/field_updates

Conversation

@clintandrewhall
Copy link
Copy Markdown
Contributor

@clintandrewhall clintandrewhall commented Sep 14, 2023

Caveat: the functional flow logic we've adopted for these components is not one I would encourage, specifically, using "drilled" onChange handlers and utilizing a composing-component-based store. Ideally, we'd use a redux store, or, at the very least a React reducer.

In the interest of time and compatibility, we've opted to use the pattern from the original components in advancedSettings. We plan to revisit the state management and prop-drilling when advancedSettings is refactored with these components.

This PR is a prerequisite to #166319 and to completing the Advanced Settings page in Serverless.

Note: Github appears to be a bit confused in the diff comparison, due to the addition to React.forwardRef... apologies for the noise.

Summary

While working on #166319 I found a number of bugs when working against actual UI settings stored in Kibana. This PR addresses those issues without waiting for the Settings page to be complete:

  • Refactors input components to have cleaner APIs, including unsavedChange and field "all the way down".
    • This cleans up confusing logic, and sets us up for Redux actions.
  • Creates a normalizeSettings function.
    • Settings returned from the UiSettingsService in an actual deployment of Kibana are drastically unpredictable. In some cases, type is missing, but value is there... or value is missing entirely, but a userValue is there.
    • This function "normalizes" the data, deriving missing parts where possible.
  • Changes the onChangeFn to accept undefined to indicate an unsaved change has been reverted, rather than relying on the value in the unsaved change.
    • This fixes a number of issues around resets and reverts.
  • Alters the unsavedChange prop to be undefined, (to indicate the lack of an unsaved change), rather than an undefined value.
  • Fixes an issue where the ImageFieldInput wasn't removing a file that had been set when resetting to default;
    • Adds an imperative ref to FieldInput components to reset a field's input beyond resetting the value, (if necessary).
  • Fixes the Storybook common setups to allow for changes to the onChange types.
  • Fixed a bug where the FieldRow was indexing an unsaved change by name, rather than by id.
  • Fixed an issue where the reset link wasn't always clearing a change to the default value.
  • Fixes an issue with the aria label not being derived properly using the query.
  • Splits the utility functions into their respective namespaces: settings and fields.
  • Adds a few more tests to the utility functions to catch logic errors.

@clintandrewhall clintandrewhall added review loe:small Small Level of Effort Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes impact:critical This issue should be addressed immediately due to a critical level of impact on the product. Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// Project:Serverless Work as part of the Serverless project for its initial release v8.11.0 labels Sep 14, 2023
@clintandrewhall clintandrewhall requested review from a team as code owners September 14, 2023 01:24
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

Copy link
Copy Markdown
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Thank you for making these fixes and additions @clintandrewhall! Overall LGTM.

I only noticed some strange behaviour in the image field but I think some of it exists from before this PR. Approving the PR since these issues could be addressed in a separate one; plus, there are no image settings in serverless.

Comment thread packages/kbn-management/settings/components/field_row/input_footer/reset_link.tsx Outdated
@clintandrewhall clintandrewhall force-pushed the advanced_settings/field_updates branch 2 times, most recently from 90120ba to cdb51c2 Compare September 19, 2023 04:08
@clintandrewhall clintandrewhall force-pushed the advanced_settings/field_updates branch from cdb51c2 to f9c5b38 Compare September 19, 2023 04:56
Copy link
Copy Markdown
Contributor

@vadimkibana vadimkibana left a comment

Choose a reason for hiding this comment

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

Codeowners changes LGTM

@kibana-ci
Copy link
Copy Markdown

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #35 / EPM Endpoints Install endpoint package install should have installed the [endpoint.metadata_current-default] transform
  • [job] [logs] FTR Configs #14 / serverless observability UI Create Case "before each" hook for "creates a case"

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/management-settings-components-field-input 1 5 +4
@kbn/management-settings-components-field-row 2 7 +5
@kbn/management-settings-utilities 30 2 -28
total -19
Unknown metric groups

API count

id before after diff
@kbn/management-settings-components-field-row 11 26 +15
@kbn/management-settings-field-definition 42 45 +3
@kbn/management-settings-types 71 75 +4
@kbn/management-settings-utilities 39 50 +11
total +33

History

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

@clintandrewhall clintandrewhall merged commit 20be3d0 into elastic:main Sep 19, 2023
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:small Small Level of Effort Project:Serverless Work as part of the Serverless project for its initial release release_note:skip Skip the PR/issue when compiling release notes review Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// v8.11.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants