-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Infrastructure UI] Adopt new saved views API #155827
[Infrastructure UI] Adopt new saved views API #155827
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
…-ref HEAD~1..HEAD --fix'
…ni/kibana into 154725-create-saved-views-hooks
…-ref HEAD~1..HEAD --fix'
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! 👏
Everything works fine - I added a comment about the delete option I think we can improve the UX there and some other questions/comments.
Another small thing I noticed when you open the react query dev tools the page layout is breaking (other pages like Alerts for example work fine)
Also the *-find
queries are displayed as disabled - should it be like that?
x-pack/plugins/infra/public/components/saved_views/manage_views_flyout.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/infra/public/pages/metrics/inventory_view/components/layout.tsx
Show resolved
Hide resolved
); | ||
} | ||
|
||
function HideableReactQueryDevTools() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's cool 👍
return ( | ||
<DeleteConfimation | ||
key={item.id} | ||
isDisabled={item.isDefault} | ||
isDisabled={item.attributes.isDefault} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Should we do this check earlier and disable the delete icon button? Maybe showing a tooltip that the default view can't be deleted if it's not clear there 🤔 When I tested I saw that I can still click it the delete icon and after that I see a disabled button:
Screen.Recording.2023-04-28.at.16.13.49.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spoke with @kkurstak in advance about this and I agree we could make a lot of improvements on the UX for saved views!
This work is however intended to only migrate the implementation for the saved views feature, we'll have the chance to further improve its usage when this work is finalized.
…s_flyout.tsx Co-authored-by: jennypavlova <jennypavlova94@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for answering the questions and adding the changes!
(added 2 small suggestions but overall it's good to go 🚀 )
Co-authored-by: jennypavlova <jennypavlova94@gmail.com>
Co-authored-by: jennypavlova <jennypavlova94@gmail.com>
It should, the requests for getting all the views is disabled as it is only necessary when managing the views, so we can trigger it on a per-event basis when the user access the views flyout and avoid the request when not necessary. |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
📓 Summary
Part of #152617
Closes #106650
Closes #154725
This PR replace the existing client React logic for handling saved views on the
infra
plugin with a new state management implementation that interacts with the newly created API.It brings the following changes:
useInventoryViews
anduseMetricsExplorerViews
custom hooks.@tanstack/react-query
for the above hooks implementation as it was already used across the plugin and simplifies the server state management. Extract the provider for the react query cache.Load views
option has been removed accordingly to the decision made with the UX team, since it wasn't adding any value that wasn't already present in theManage views
option.Even if we are duplicating similar logic to handle the Inventory and Metrics Explorer views, we decided to keep them separated to easily control their behaviour and avoid coupled logic that can be painful to split in future.
🐞 Bug fixes
This implementation also fixed some existing bugs in production:
It has been fixed and the selected view fallbacks to the default view.
👨💻 Review hints
The best way to verify all the interactions and loadings work correctly as a user expects, running the branch locally with an oblt cluster is recommended.
In both the inventory and metrics explorer pages, the user should be able to:
👣 Following steps