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.

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

[ObsUX] Use bottom bar action buttons in Infra settings #169210

Closed
mykolaharmash opened this issue Oct 18, 2023 · 10 comments · Fixed by #175024
Closed

[ObsUX] Use bottom bar action buttons in Infra settings #169210

mykolaharmash opened this issue Oct 18, 2023 · 10 comments · Fixed by #175024
Assignees
Labels
enhancement New value added to drive a business result Feature:Metrics UI Metrics UI feature good first issue low hanging fruit Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team

Comments

@mykolaharmash
Copy link
Contributor

mykolaharmash commented Oct 18, 2023

At the moment Advanced Settings, APM and Profiling plugins use the same bottom bar with "Save"/"Cancel" buttons on their settings screens which is different from what Infra Settings is using. We need to unify the UI and, ideally create a shared component for that bottom bar as currently all three plugins use custom components that look the same.

Image

@botelastic botelastic bot added the needs-team Issues missing a team label label Oct 18, 2023
@mykolaharmash mykolaharmash added the Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services label Oct 18, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@botelastic botelastic bot removed the needs-team Issues missing a team label label Oct 18, 2023
@cauemarcondes cauemarcondes added the good first issue low hanging fruit label Oct 18, 2023
@cauemarcondes
Copy link
Contributor

@mykolaharmash I added a good first issue label, as this would be a nice starting issue for our new team to start working on Infra.

@abfahimb
Copy link

@cauemarcondes Is it ok to new dev work on this issue? if ok can you please help me to assign this issue. Thanks.

@cauemarcondes
Copy link
Contributor

cauemarcondes commented Oct 23, 2023

Hey @abfahimb, one example of how to implement this would be following what has done on APM.

The settings page on APM imports the BottomBarActions component to show the black bar like in the description. Here is the file: https://github.com/elastic/kibana/blob/main/x-pack/plugins/apm/public/components/app/settings/agent_configurations/agent_configuration_create_edit/settings_page/settings_page.tsx#L194-L203.

We don't want to copy and paste this component in the Infra plugin but rather export it to a common place where more plugins could use it.

So here are the steps:

  1. Export BottomBarActions component from APM plugin to observability_shared plugin https://github.com/elastic/kibana/tree/main/x-pack/plugins/observability_shared/public/components/
  2. Delete the current BottomBarActions component from APM and use the newly created component from observability_shared
  3. Remove the current implementation from Infra
  4. Use the BottomBarActions component on Infra

@mykolaharmash could you point out where the files are in the Infra plugin?

@mykolaharmash
Copy link
Contributor Author

Hey @cauemarcondes @abfahimb. Here is the component that handles the bottom bar in Infra.

Just a heads up, bringing this unified bottom bar to Infra might be a bit trickier as it does not already use a similar component to APM or Advanced Settings, we might need to refactor the whole Infra settings form state management a bit.

@smith smith added the enhancement New value added to drive a business result label Oct 30, 2023
@smith smith added Feature:Metrics UI Metrics UI feature Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Nov 9, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@smith smith removed the Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services label Nov 9, 2023
@jennypavlova jennypavlova self-assigned this Jan 15, 2024
@mykolaharmash
Copy link
Contributor Author

@jennypavlova just fyi, there is an open PR that also touches some parts of the settings editing logic, there might be some conflicts.

@jennypavlova
Copy link
Member

@mykolaharmash Thank you, I will keep it in mind, I am working on other PR/review so I was only looking at the current implementation for now and I haven't pushed any changes yet and I should probably use part of the changes in the PR you mentioned so if the PR is merged soon I will pull the changes 👍

@jennypavlova
Copy link
Member

jennypavlova commented Jan 17, 2024

The PR is not merged yet so I started with some changes here: I was looking at the infra settings and as @mykolaharmash mentioned they are different than the form in APM - the on/off feature switches are pretty similar but the indices and name field are different - there are no default values concept and there is extra validation on the metrics indices and the name should not be empty.

Image

I checked in APM and there is no messages/validation there if I set an invalid value the save button is visible but when I click it the value is reset (Which I think is not perfect so we should keep the infra validation even if the bar is hidden when the form is invalid):

Screen.Recording.2024-01-17.at.14.11.29.mov

So in infra when the form is invalid, the action bar will disappear and also if there is a change in one of the fields and then the change is removed and the old value is shown in the form it will still track it as change. So I tried it and apart from that the infra functionality is the same as it was with the old buttons (except that the bar is hidden and the buttons are disabled).

infra_settings_new_action_bar_720.mov

I found the defaultSourceConfiguration but in some cases, it can be merged with other values so currently we don't have reset to default option (as this will override other configuration the user set there) So, in that case, I don't think we need the Reset to default action.

@mykolaharmash I saw you mentioned something about the state management in infra - do you mean the default values and validation (I will look into those) or I am missing something there - I am adding a draft PR so you can see what I mean in this comment

@mykolaharmash
Copy link
Contributor Author

@jennypavlova tbh I don't quite remember, but I think I was just meant the fact that component props and callbacks from the custom bottom bar in Infra are quite different from the one in APM in Advanced Settings, and we might need more changes in Infra to adapt to the unified bottom bar API.

jennypavlova added a commit that referenced this issue Jan 22, 2024
Closes #169210 
## Summary

This PR adds bottom bar action buttons to the infra settings page to
make it consistent with the APM settings page. The bottom bar appears
after a change is made (and won't appear if the value is the same as it
was before - same as APM).

| APM | Infra Now |
| ---- | ---------- |
|
![image](https://github.com/elastic/kibana/assets/14139027/8400cc75-c3d3-40c6-adf7-040779598c7d)
|
![image](https://github.com/elastic/kibana/assets/14139027/60f49a78-f733-43f4-a1c8-9b9c337b49f6)
|

The infra page validation is a bit different so I used the same logic as
in advance settings - if the field is invalid disable the button and
show a tooltip:

<img width="1920" alt="image"
src="https://github.com/elastic/kibana/assets/14139027/9e73e77f-f572-4dbe-a8cc-9aa7bf5ec370">

## Testing 

- Open the infra settings page
- Check if the bottom bar is displayed and the save/discard
functionality works as expected
   - Check infra valdation
- Verify that the APM settings page works as expected



https://github.com/elastic/kibana/assets/14139027/6804e24b-2b33-456d-be8f-cb17cc044759
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
…c#175024)

Closes elastic#169210 
## Summary

This PR adds bottom bar action buttons to the infra settings page to
make it consistent with the APM settings page. The bottom bar appears
after a change is made (and won't appear if the value is the same as it
was before - same as APM).

| APM | Infra Now |
| ---- | ---------- |
|
![image](https://github.com/elastic/kibana/assets/14139027/8400cc75-c3d3-40c6-adf7-040779598c7d)
|
![image](https://github.com/elastic/kibana/assets/14139027/60f49a78-f733-43f4-a1c8-9b9c337b49f6)
|

The infra page validation is a bit different so I used the same logic as
in advance settings - if the field is invalid disable the button and
show a tooltip:

<img width="1920" alt="image"
src="https://github.com/elastic/kibana/assets/14139027/9e73e77f-f572-4dbe-a8cc-9aa7bf5ec370">

## Testing 

- Open the infra settings page
- Check if the bottom bar is displayed and the save/discard
functionality works as expected
   - Check infra valdation
- Verify that the APM settings page works as expected



https://github.com/elastic/kibana/assets/14139027/6804e24b-2b33-456d-be8f-cb17cc044759
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Metrics UI Metrics UI feature good first issue low hanging fruit Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants