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

Add config option to map values to colors #4894

Merged
merged 6 commits into from
Sep 16, 2015

Conversation

lukasolson
Copy link
Member

Closes #4817.

This PR adds a config option that allows users to associate colors with specific values, and defaults Count to be the standard green, so that it's not mapped to different colors based on the context of the application.

@BigFunger
Copy link
Contributor

A couple of things:

  • This doesn't maintain a unique list of colors for a visualization.
    • I took a visualization, and found out the color that was being used by default for the second line.
    • I added a config value to make that the color for the first line.
    • The visualization now uses that color for both lines.
      colors
  • this doesn't watch for changes in config, so the user must do a hard refresh to apply the colors
  • the variable name colorMapping for the config values was confusing to me in the context of the code when there is already a mappedColors variable. Purely subjective.

@lukasolson
Copy link
Member Author

Okay, so I made some pretty drastic changes.

I moved the logic that maps colors from color.js to mapped_colors.js. I also modified the latter file to return a singleton instead of a class that needs to be instantiated (I anticipate that only one instance will ever be needed). Prior to these changes, mapped_colors was essentially just a set, but now it has some important functionality that checks the colors mapped by the user's config, and picks colors that the user hasn't already used in the config.

@lukasolson lukasolson assigned BigFunger and unassigned lukasolson Sep 11, 2015
@rashidkpc rashidkpc added v4.3.0 and removed v4.2.0 labels Sep 14, 2015
@rashidkpc rashidkpc assigned tsullivan and unassigned BigFunger Sep 14, 2015
@rashidkpc rashidkpc added v4.2.0 and removed v4.3.0 labels Sep 14, 2015
color = getColors(arr);
}));

afterEach(ngMock.inject(function (config) {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick - kind of inconsistent here in that you use fat arrow syntax in other new code

@tsullivan
Copy link
Member

LGTM - other than those 2 things

@lukasolson lukasolson assigned rashidkpc and unassigned tsullivan Sep 15, 2015
@lukasolson
Copy link
Member Author

Passing to @rashidkpc for a quick final look.

@rashidkpc
Copy link
Contributor

This LGTM, it also provides a work around for users that need custom color, albeit global, until custom color palette support is in. Eg, setting success to green, errors to red

screen shot 2015-09-16 at 7 38 01 am

rashidkpc added a commit that referenced this pull request Sep 16, 2015
Add config option to map values to colors
@rashidkpc rashidkpc merged commit aa1511b into elastic:master Sep 16, 2015
@seshukumaradiraju
Copy link

Tried this and have one issue.
Gave Success color as green and error as red.

When data has both categories .. the colors are shown correctly.
image

But when I zoom in where only errors are present, color changes to green.
image

Any fix/workaround for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants