O3-1015 Implementer should be able to configure primary color #272
Conversation
File size impactMerging config-brand into master impact files as follow: @openmrs/esm-devtools-app (+5.16%)
@openmrs/esm-implementer-tools-app (+1.05%)
@openmrs/esm-login-app (+1.15%)
@openmrs/esm-offline-tools-app (+1.54%)
@openmrs/esm-primary-navigation-app (+1.33%)
@openmrs/esm-app-shell (+0.05%)
|
There was a problem hiding this comment.
I think that this is basically the right approach. The main downside is that we're requiring this slightly verbose pattern wherever this colour is used.
This might be more invasive, but maybe we could leverage a Sass mixin, e.g.,
@mixin brand-background-color {
background-color: $brand-01;
background-color: var(--brand-01);
}Then again, maybe that's just making more trouble?
| // background-color: $brand-01; | ||
| background-color: var(--brand-01); | ||
| // border-bottom: $brand-02; | ||
| border-bottom: var(--brand-02); |
There was a problem hiding this comment.
Clean-up: we should probably keep the commented-out lines.
| :root { | ||
| --brand-01: #005d5d; | ||
| --brand-02: #004144; | ||
| --brand-teal-01: #007d79; | ||
| } |
There was a problem hiding this comment.
Since this is in an scss file, wouldn't it make sense to do:
| :root { | |
| --brand-01: #005d5d; | |
| --brand-02: #004144; | |
| --brand-teal-01: #007d79; | |
| } | |
| :root { | |
| --brand-01: $brand-01; | |
| --brand-02: $brand-02; | |
| --brand-teal-01: $brand-teal-01; | |
| } |
There was a problem hiding this comment.
Secondary point to all of this, but brand-teal-01 could use a rename...
There was a problem hiding this comment.
Strangely, that doesn't work—if you do this then the variable --brand-01 gets set to the string literal "$brand-01".
There was a problem hiding this comment.
Secondary point to all of this, but brand-teal-01 could use a rename...
Agreed. brand-01 is lighter than brand-02, and brand-teal-01 is lighter than brand-01. We can either call it brand-03 and not worry about the lightness, or we can call it brand-light-01 (and thereby defer the problem until there are two new brand colors to add). Other suggestions welcome.
There was a problem hiding this comment.
Hmmm... I think brand-light-01 at least gets the goal across better? I'm thinking that we'll definitely need some sort of documentation with annotated screenshots so people can see where the branding takes place.
|
If we're going to use a mixin I'd say go all the way and use this one: @mixin var($property, $varName) {
#{$property}: map-get($vars, $varName);
#{$property}: var(--#{$varName});
}which is used like @include var(background-color, brand-01);However, I'm inclined to think that people outside the squad would rarely use this strange syntax. Unless—and this is something I think we should do—we get rid of the |
brandones
changed the title
Yeah... Sass mixins don't make for easy-reading syntax. I was just trying to come up with a way to avoid copy-and-paste where we end up with just the
I don't necessarily disagree with this, but it needs some careful working through since at least some of the variables we define seem to be intended as overrides of Carbon-defined variables (though at a quick pass, it doesn't look like we're actually using different values).
Or the worst-case scenario |
https://issues.openmrs.org/browse/O3-1015