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

AliasColors allows only hardcoded keys #1

Closed
cmur2 opened this issue Nov 1, 2019 · 8 comments
Closed

AliasColors allows only hardcoded keys #1

cmur2 opened this issue Nov 1, 2019 · 8 comments
Labels
bug Something isn't working

Comments

@cmur2
Copy link
Collaborator

cmur2 commented Nov 1, 2019

I'm not entirely sure if I understood it correctly but isn't https://github.com/spoved/crafana.cr/blob/master/src/crafana/models/alias_colors.cr supposed to support arbitrary key value pairs to map colors to the targets with a certain name in the legend?

Would it be possible to extend this?

@cmur2
Copy link
Collaborator Author

cmur2 commented Nov 1, 2019

I currently solved this in my fork like this cmur2@6f2d065 which works but maybe there is a more elegant solution @kalinon ?

@kalinon
Copy link
Collaborator

kalinon commented Nov 2, 2019

Let me review. I never really used alias colors that much so ill see if i can find anything in the api documentation

@cmur2
Copy link
Collaborator Author

cmur2 commented Nov 2, 2019

Thanks for that and also your effort in building this shard, it's very well built and useful!

If it helps this is a real world snippet from one of my dashboards using alias colors:

{
  "panels": [
    {
      "aliasColors": {
        "idle": "#ffcc00",
        "iowait": "#330099",
        "nice": "#ff8000",
        "softirq": "#aa2b00",
        "system": "#00cc00",
        "user": "#0066b3"
      },
      "bars": false,
      "dashLength": 10
    }
  ]
}

@kalinon
Copy link
Collaborator

kalinon commented Nov 2, 2019

Perfect, I dont see anything in the official API docs for it.

How about possibly changing it to:

Hash(String, String | Crafana::RGBA | Crafana::RGB)

We could even ditch the AliasColors object or alias it to the hash.

alias AliasColors = Hash(String, String | Crafana::RGBA | Crafana::RGB)

@kalinon
Copy link
Collaborator

kalinon commented Nov 2, 2019

I should probably also make an issue to make color parsing easier. So if people want to use hex colors they can, or whatever format they want. If we do String | Crafana::RGBA | Crafana::RGB it might actually break things on de-serialization.

@kalinon kalinon added the bug Something isn't working label Nov 2, 2019
@cmur2
Copy link
Collaborator Author

cmur2 commented Nov 2, 2019

I think dropping AliasColors object might make sense as long as we can avoid breaking changes in color parsing.

@kalinon
Copy link
Collaborator

kalinon commented Nov 2, 2019

I think dropping AliasColors object might make sense as long as we can avoid breaking changes in color parsing.

Agreed. Perhaps adding a Hex color object and abstracting the json decode into a parent module. We can merge the Hash(String, String) in the meantime if youd like to get it into master.

@cmur2
Copy link
Collaborator Author

cmur2 commented Nov 2, 2019

I will open a PR for the Hash(String, String) change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants