-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Convert button group to toggle. Refactor code structure for scalability. #47083
Conversation
Pinging @elastic/infra-logs-ui (Team:infra-logs-ui) |
💚 Build Succeeded |
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.
@phillipb would you mind adding some screenshots (before/after), so it's easier to understand the scope of this? :)
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.
Looks good, really like the change visually 👍 I've left some comments r.e. possibly cutting out some duplication.
x-pack/legacy/plugins/infra/public/components/inventory/toolbars/container_toolbar.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/public/components/inventory/toolbars/container_toolbar.tsx
Outdated
Show resolved
Hide resolved
I'm slightly concerned that Docker, Kubernetes etc are now rather well hidden below what looks like a "Hosts" dropdown. Users may think that clicking that dropdown allows them to select a single host, rather than allow them to select a different view of their inventory. I wonder if it would be better to adopt the approach used by other the dropdowns here, and prefix the text with the kind of thing the dropdown relates to (for example, "Metrics: CPU Usage"), so the dropdown tells you both the kind of thing you are selecting, and the selection you have made. So perhaps something like "View: Hosts"? Although perhaps there may be a better word than "View" as we also use "View" below in "Table view" and "Map view"? But I think some kind of informational prefix on the dropdown is needed to avoid confusion. Is there a general term we use elsewhere to refer to the collection of things like "Hosts", "Docker", "Kubernetes" etc . . . . ? PS, typo in Kubernetes too, in your screenshot. |
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 made some commments inline about reducing some of the boiler plate in the toolbars. I think we should also take this opportunity to change the styles so they look more consistent. Here is what they should look like
This is a screen shot from 8721a2d based on design provided by @hbharding. The "view" drop down should be positioned like you have in this PR but the "Metric" and "Group By" drop downs should look like the image above.
x-pack/legacy/plugins/infra/public/components/inventory/toolbars/host_toolbar.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/public/components/waffle/waffle_inventory_switcher.tsx
Outdated
Show resolved
Hide resolved
💔 Build Failed |
💚 Build Succeeded |
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
…ty. (elastic#47083) * Convert button group to toggle. Refactor code structure for scalability. * Remove duplicated code and reduce boilerplate * Properly internationalize invotory label * Reduce more boilerplate
Summary
This PR restructures the inventory page's UI, and the app's structure to support more types of inventory.
For the UI:
For the Code
Fixes: #45924.
Before
After
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Documentation was added for features that require explanation or tutorialsUnit or functional tests were updated or added to match the most common scenariosThis was checked for keyboard-only and screenreader accessibilityFor maintainers