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

Jump to bottom

[Fleet] Implement Settings new design for fleet server hosts #118385

Merged
merged 7 commits into from
Nov 15, 2021
Merged

[Fleet] Implement Settings new design for fleet server hosts #118385

merged 7 commits into from
Nov 15, 2021

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Nov 11, 2021 •
edited
Loading

Summary

More progress on #117317

In the effort to refacto the new settings tab, that PR implement the new design for the fleet server hosts section.

Done in that PR:

  • I tryed to have components the more pure possible, and all the big chunk of logic are extract to their own hooks. Every component should be visible in storybook.
  • New url for the edit fleet server hosts flyout
  • I created a new hook for the confirm modal in the settings tab (will be reused for the outputs CRUD)

Todo

  • unit test form hooks

UI Changes

Screen Shot 2021-11-11 at 11 21 56 AM

Screen Shot 2021-11-11 at 11 23 59 AM

Screen Shot 2021-11-11 at 11 24 03 AM

@nchaulet nchaulet added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 labels Nov 11, 2021
@nchaulet nchaulet self-assigned this Nov 11, 2021
@nchaulet nchaulet changed the title [Fleet] Implement Settings new design for fleet server hots [Fleet] Implement Settings new design for fleet server hosts Nov 11, 2021
@nchaulet nchaulet force-pushed the feature-settings-new-design branch from 14d5931 to 45bfde4 Compare November 11, 2021 16:45
nchaulet
nchaulet commented Nov 11, 2021
View reviewed changes
...t/public/applications/fleet/sections/settings/components/fleet_server_hosts_flyout/index.tsx
}) => {
const { docLinks } = useStartServices();

const form = useFleetServerHostsForm(fleetServerHosts, onClose);
Copy link
Member Author

Choose a reason for hiding this comment

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

@kpollich @joshdover I am wondering if this is good enough for separating business and presentation logic or if we should push this further and have a container component that calls that hooks and have really pure presentation component and pass this as props to this component?

Copy link
Member

Choose a reason for hiding this comment

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

Personally I'm in favor of the approach you've already implemented here. I don't think a container component would add much value in this particular case, as it'd mostly just serve to pass the form data returned from this hook down into the child component.

I was looking at the synthetics UI extensions for Fleet recently and I thought the component/hook patterns were well done. https://github.com/elastic/kibana/tree/main/x-pack/plugins/uptime/public/components/fleet_package

@nchaulet nchaulet force-pushed the feature-settings-new-design branch from 45bfde4 to ee48337 Compare November 11, 2021 19:13
@nchaulet nchaulet marked this pull request as ready for review November 15, 2021 13:41
@nchaulet nchaulet requested a review from a team as a code owner November 15, 2021 13:41
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@nchaulet nchaulet force-pushed the feature-settings-new-design branch from dd04102 to a904559 Compare November 15, 2021 13:49
@elastic elastic deleted a comment from kibanamachine Nov 15, 2021
@nchaulet nchaulet force-pushed the feature-settings-new-design branch from a904559 to 02cd376 Compare November 15, 2021 14:12
@nchaulet nchaulet requested a review from kpollich November 15, 2021 14:12
kpollich
kpollich approved these changes Nov 15, 2021
View reviewed changes
Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Ran through everything locally and LGTM besides a few very minor typos/nits. 🚀

...s/fleet/public/applications/fleet/sections/settings/components/hosts_input/index.stories.tsx Show resolved Hide resolved
.../fleet/sections/settings/components/fleet_server_hosts_flyout/use_fleet_server_host_form.tsx Outdated Show resolved Hide resolved
x-pack/plugins/fleet/public/applications/fleet/sections/settings/hooks/use_confirm_modal.tsx Outdated Show resolved Hide resolved
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #11 / detection engine api security and spaces enabled update_actions updating actions should be able to create a new webhook action and attach it to a rule with a meta field and run it correctly

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fleet 526 531 +5

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
fleet 1122 1123 +1

Async chunks

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

id before after diff
fleet 616.3KB 621.2KB +5.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 104.4KB 104.6KB +144.0B
Unknown metric groups

API count

id before after diff
fleet 1224 1225 +1

History

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

cc @nchaulet

@nchaulet nchaulet merged commit df45f43 into elastic:main Nov 15, 2021
@nchaulet nchaulet deleted the feature-settings-new-design branch November 15, 2021 17:56
@kibanamachine kibanamachine mentioned this pull request Nov 15, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 15, 2021
@nchaulet @kibanamachine
[Fleet] Implement Settings new design for fleet server hosts (elastic…
74eeea6 
…#118385)
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 15, 2021
@kibanamachine @nchaulet
[Fleet] Implement Settings new design for fleet server hosts (#118385) (
d178666 
#118585)

Co-authored-by: Nicolas Chaulet <[email protected]>
mbondyra pushed a commit to mbondyra/kibana that referenced this pull request Nov 19, 2021
@nchaulet @mbondyra
[Fleet] Implement Settings new design for fleet server hosts (elastic…
00df969 
…#118385)
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
@nchaulet @dmlemeshko
[Fleet] Implement Settings new design for fleet server hosts (#118385)
8066b0a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpollich kpollich kpollich approved these changes

Assignees

@nchaulet nchaulet

Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.0.0 v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nchaulet @elasticmachine @kibana-ci @kibanamachine @kpollich