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

UI-272 Add warning class to radial chart in stencil library Signed-of… #359

Merged
merged 3 commits into from
May 17, 2019

Conversation

tarablack01
Copy link
Contributor

@tarablack01 tarablack01 commented May 15, 2019

…f-by: Tara Black [email protected]

🔩 Description

Add warning class to radial chart in stencil library.

👍 Definition of Done

Screen Shot 2019-05-16 at 8 17 12 PM

⛓️ Related Resources

https://chefio.atlassian.net/browse/A2-792

✅ Checklist

  • [x ] Necessary docs added/updated?

@tarablack01 tarablack01 force-pushed the UI-272-chef-radical-chart-stencil branch from 6dab387 to f284f7b Compare May 15, 2019 22:30
@susanev
Copy link
Contributor

susanev commented May 16, 2019

we are trying to move away from hsl, because the hex(1) -> hsl -> hex(2) conversion results in different hex values for (1) and (2) which makes it more difficult for the ux team to collaborate with the engineers. is there any way we could change this component to start using hex instead of hsl in this pr?

@tarablack01 tarablack01 force-pushed the UI-272-chef-radical-chart-stencil branch from f284f7b to ddb2e4c Compare May 16, 2019 14:15
@tarablack01
Copy link
Contributor Author

@susanev Updated PR to replace HSL with HEX :)

@apriofrost
Copy link

Hi Tara! Thanks for working on the issue!
The comment is related to the color palette change work planned for the future. I noticed that the warning color is using chef-primary, we tried to stay away from using this color because it looks very similar to chef-critical when they are near each other in the chart.

The proposed colors for the mini donut chart are:

  • chef-critical for critical services count
  • #FF9654 (new color to be added to the palette) for warning services count
  • #3DA5FF (new color to be added to the palette) for OK services count
  • #B7BCBC (new color to be added to the palette) for unknown services count.

Last time when we chatted in the UI team backlog grooming, we decided to add the colors for now without turning them into variables. There is a spike (#309) planned for the color palette work (#275). But I'm open to other suggestions you might have on how we implement these color changes. Thank you! :)

Copy link
Contributor

@scottopherson scottopherson left a comment

Choose a reason for hiding this comment

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

If we're removing all usages of chef-hsl in this PR we should go ahead and remove the function definitions as well:

sass({
functions: {
'chef-hsl($hueSatLight)': function (hsl) {
return new sassLib.types.String(`hsl(${hsl.getValue()})`);
},
'chef-hsla($hueSatLight, $alpha: 1)': function (hsl, alpha) {
return new sassLib.types.String(`hsla(${hsl.getValue()}, ${alpha.getValue()})`);
}
}
})

👍

@tarablack01 tarablack01 force-pushed the UI-272-chef-radical-chart-stencil branch from ddb2e4c to fa1686c Compare May 17, 2019 01:12
@tarablack01
Copy link
Contributor Author

@apriofrost I've added the new colors to be used within the chart components and updated the examples to utilize the new chart colors. I've also picked up story #275 to implement the color palette work when it's ready.
@scottopherson The unused hsl, hsla functions have been removed.

@susanev
Copy link
Contributor

susanev commented May 17, 2019

@tarablack01 we had intended that #309 would be done before #275 so we can get an idea of the amount of engineering effort will be involved.

@vjeffrey vjeffrey merged commit 58632b3 into master May 17, 2019
@chef-ci chef-ci deleted the UI-272-chef-radical-chart-stencil branch May 17, 2019 16:08
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