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

[Graph] Field manager #45384

Merged
merged 162 commits into from
Sep 24, 2019
Merged

[Graph] Field manager #45384

merged 162 commits into from
Sep 24, 2019

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Sep 11, 2019

Summary

This PR moves the field manager to react and EUI. It also implements the state handling as redux store.

Adding new fields

I made it possible to add multiple fields at once because in most cases you want to work with more than one field.
Screenshot 2019-09-20 at 16 05 33

Field editor

Max hops is moved to a collapsed accordion because in most cases you don't change this setting. If you change the field, the current field is deselected and the new field is selected and gets the settings from the current field (color, icon and hop size). Changes are directly applied (no update button), because they are easy to reverse - even if you remove a field you can just add it again and get all settings back.
Screenshot 2019-09-20 at 16 05 39
Screenshot 2019-09-20 at 16 05 45

Disabled fields

Fields can be disabled by the switch in the field editor or by shift-clicking the field. Not really happy with the style of disabled fields yet.
Screenshot 2019-09-20 at 16 07 12

State handling

For now the redux store doesn't do much, but it will be extended in subsequent PRs to do all the state handling currently living in legacy JS files.

@flash1293
Copy link
Contributor Author

@cchaos I like it being consistent with filters - I updated the PR, could you take another look? While I was at it, I changed the default field colors to use the light colors of the eui palette. I have the same problem as maps that combo box can't show an icon as selected option.

@cchaos
Copy link
Contributor

cchaos commented Sep 20, 2019

Design PR4U: flash1293#4

@cchaos
Copy link
Contributor

cchaos commented Sep 20, 2019

I have the same problem as maps that combo box can't show an icon as selected option.

That's fine for now, I'll look for or create a ticket for this type of display in EUI.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cchaos
Copy link
Contributor

cchaos commented Sep 20, 2019

I also noticed that it seemed like the field edits were not trickling down to the Graph itself?

@flash1293
Copy link
Contributor Author

@cchaos the disabling thing is expected, disabling the field just prevents new nodes of this type to be discovered via searches and expands. The color should get updated, good catch - I will look into this.

@flash1293
Copy link
Contributor Author

@cchaos I couldn't reproduce the problem with settings not trickling down to the graph - it's working fine for me. Could you check whether it's reproducible?
change_field_settings

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cchaos
Copy link
Contributor

cchaos commented Sep 23, 2019

@flash1293 Hmmm, I pulled down the lates and I'm not having that issue anymore. I wonder if it was because I was working off an already created Graph (in a different version) but since I've started new, it's been working. I'll keep an eye out for if it happens again.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

The UI and behavior LGTM. Just had a couple comments about re-usability. Also, now that a user can choose any color, we really need to use the light or dark functionality to change the icon color in the graph itself.

}

function getIconForDataType(dataType: string) {
const icons: Partial<Record<string, UnwrapArray<typeof ICON_TYPES>>> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like you're doing this same thing in Lens. Maybe you'll want to figure out a more global service?

export function getColorForDataType(type: string) {
const iconType = getIconForDataType(type);
const { colors } = palettes.euiPaletteColorBlind;
const colorIndex = stringToNum(iconType) % colors.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should definitely make sure we're using the same colors across all apps.

@flash1293
Copy link
Contributor Author

flash1293 commented Sep 24, 2019

The UI and behavior LGTM. Just had a couple comments about re-usability. Also, now that a user can choose any color, we really need to use the light or dark functionality to change the icon color in the graph itself.

I will take care of that when moving the visualization itself over.

Seems like you're doing this same thing in Lens. Maybe you'll want to figure out a more global service?

We definitely should - Discover is another possible consumer. #46450

@flash1293 flash1293 merged commit 2b866be into elastic:master Sep 24, 2019
@flash1293 flash1293 deleted the graph/field-manager branch September 24, 2019 12:34
flash1293 added a commit to flash1293/kibana that referenced this pull request Sep 24, 2019
flash1293 added a commit that referenced this pull request Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Graph Graph application feature release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants