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

[Infra UI] Custom Field Grouping for Waffle Map #28949

Merged
merged 9 commits into from
Jan 30, 2019
Merged

[Infra UI] Custom Field Grouping for Waffle Map #28949

merged 9 commits into from
Jan 30, 2019

Conversation

simianhacker
Copy link
Member

@simianhacker simianhacker commented Jan 17, 2019 •
edited by zube bot
Loading

This PR closes #26331 by adding support for grouping by a custom field in the waffle map. Currently the custom field doesn't persist beyond the current session, this is a good first start. We can look into way to persist these "adhoc" fields in the future.

custom_field_demo

Here is a closer look at the form for the custom fields

image

@elasticmachine
Copy link
Contributor

Pinging @elastic/infrastructure-ui

@simianhacker simianhacker mentioned this pull request Jan 17, 2019
Closed
@elasticmachine
Copy link
Contributor

💔 Build Failed

@simianhacker simianhacker self-assigned this Jan 17, 2019
@simianhacker simianhacker mentioned this pull request Jan 17, 2019
Closed
@honzakral
Copy link
Contributor

Awesome, this is exactly what I had in mind in #26331 (comment), thank you, @simianhacker !

@exekias
Copy link
Contributor

exekias commented Jan 17, 2019

This is great @simianhacker 😍, just a few comments:

  • Label is optional (you use the field name when it's not provided). I like that, would be nice to make it clear in the form, by showing something like (Optional) in the description
  • I see you update he URL when adding a field to the grouping list, but something goes wrong when you load the page from the URL, they are not populated back in the drop down (although they seem to be used).
  • Custom fields are not stored anywhere, I think this is good for an initial release. Do you plan to "remember" previously used custom fields, or the UI is quick enough to be good as it is?

Would love @makwarth opinion on this PR

@elasticmachine
Copy link
Contributor

💔 Build Failed

@simianhacker
Copy link
Member Author

simianhacker commented Jan 17, 2019 •
edited
Loading

  • Label is optional (you use the field name when it's not provided). I like that, would be nice to make it clear in the form, by showing something like (Optional) in the description

Looks like I beat you to this

  • I see you update he URL when adding a field to the grouping list, but something goes wrong when you load the page from the URL, they are not populated back in the drop down (although they seem to be used).

Just push a fix that serializes custom options to the URL so they persist between reloads

  • Custom fields are not stored anywhere, I think this is good for an initial release. Do you plan to "remember" previously used custom fields, or the UI is quick enough to be good as it is?

I agree I think what we have now makes a nice MVP that we can extend into the future to serialize custom fields to Kibana. We might want to do that via the Settings UI that @weltenwort is working on.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@makwarth
Copy link

Few comments:

  • How about moving the search field to the top of the dropdown, as opposed to its own dropdown menu item? Fast access!
  • Do we need the optional Label field at all? Let's just write field.name in the dropdown? It's just simpler. (I agree that when this gets hooked up to the config UI, the custom label name makes a lot of sense.)
  • This is awesome! :)

@exekias
Copy link
Contributor

exekias commented Jan 18, 2019

++ on what @makwarth proposes, I think that should be super quick for the user, I like that

@simianhacker
Copy link
Member Author

simianhacker commented Jan 18, 2019 •
edited
Loading

@makwarth I made the changes to the order and form:

image

image

Update: Holy crap those images are huge!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@makwarth
Copy link

@simianhacker I actually meant something like this :-)

51394811-89587c80-1af8-11e9-8a1d-acfdd6029357-2

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker
Copy link
Member Author

FYI... @makwarth and I discussed it and we are going to defer adding the custom fill pull down directly in the context menu till later since it will require more work. We would like to get this in as a quick win.

@ryankeairns ryankeairns mentioned this pull request Jan 23, 2019
Closed
@alvarolobato alvarolobato added the Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services label Jan 29, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@skh skh self-assigned this Jan 30, 2019
@skh
Copy link
Contributor

skh commented Jan 30, 2019

On IE11 /Windows 10 (in Browserstack), clicking the x here to remove a grouping does not work as intended, it opens the dropdown menu instead:

image

The grouping can still be removed by selecting it from the dropdown again. If this is easily fixable this would be nice to have.

Other than that, looks great 👍

@simianhacker
Copy link
Member Author

@skh Let me see if there is a quick fix for that IE bug. I'm pretty sure that's been there the whole time. If it's more than a one line fix I will open a seperate PR.

@simianhacker
Copy link
Member Author

@skh I'm going to defer the IE issue, We are using EUI components and the nesting of the EuiBadge with iconOnClick inside the EuiFilterButton is causing the event to not propagate properly. I created an issue to track it #29627. Once this PR goes green I will merge it.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker simianhacker merged commit c66363a into elastic:master Jan 30, 2019
simianhacker added a commit to simianhacker/kibana that referenced this pull request Jan 30, 2019
@simianhacker
[Infra UI] Custom Field Grouping for Waffle Map (elastic#28949)
a2885e3 
* [Infra UI] Add Support for Grouping By Custom Field

* fixiing typescript errors

* Serializing custom options to url so they persist accross reloads

* Fixing more errors

* removing label; moving custom field to top of menu

* fixing typescript error

* Adding intl formatMessage to strings
@simianhacker simianhacker mentioned this pull request Jan 30, 2019
Merged
simianhacker added a commit that referenced this pull request Jan 30, 2019
@simianhacker
[Infra UI] Custom Field Grouping for Waffle Map (#28949) (#29636)
dc54aca 
* [Infra UI] Add Support for Grouping By Custom Field

* fixiing typescript errors

* Serializing custom options to url so they persist accross reloads

* Fixing more errors

* removing label; moving custom field to top of menu

* fixing typescript error

* Adding intl formatMessage to strings
@simianhacker simianhacker mentioned this pull request Mar 4, 2019
Closed
@simianhacker simianhacker deleted the group_by_custom branch April 17, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skh skh skh approved these changes

@weltenwort weltenwort Awaiting requested review from weltenwort

@Kerry350 Kerry350 Awaiting requested review from Kerry350

Assignees

@simianhacker simianhacker

@skh skh

Labels
Feature:Metrics UI Metrics UI feature review Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v6.7.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Group by more fields in Infrastructure waffle map view
7 participants
@simianhacker @elasticmachine @honzakral @exekias @makwarth @skh @alvarolobato