-
Notifications
You must be signed in to change notification settings - Fork 54
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
[OUDS] Add the Bootstrap compatibilities in the doc and the code #2833
base: ouds/main-lmp-tokens-colors
Are you sure you want to change the base?
[OUDS] Add the Bootstrap compatibilities in the doc and the code #2833
Conversation
2458723
to
c62291e
Compare
98f2509
to
6a2b3d5
Compare
75bb0fc
to
2120289
Compare
664b27f
to
f5fae42
Compare
2120289
to
c1477b0
Compare
text-align: var(--#{$prefix}body-text-align); | ||
|
||
/* rtl:remove */ | ||
letter-spacing: var(--#{$prefix}body-letter-spacing); // OUDS mod | ||
background-color: var(--#{$prefix}body-bg); // 2 | ||
background-color: var(--#{$prefix}color-bg-primary); // 2 // OUDS mod: instead of `var(--#{$prefix}body-bg)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure about these two changes in body
and the following in the file, maybe
background-color: var(--#{$prefix}color-bg-primary); // 2 // OUDS mod: instead of `var(--#{$prefix}body-bg)` | |
background-color: var(--#{$prefix}body-bg, var(--#{$prefix}color-bg-primary)); // 2 // OUDS mod: instead of `var(--#{$prefix}body-bg)` |
would be better ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand, you want to do that to make it possible to change it through body-bg
like its meant to be in bootstrap ? It would be taken into account even if not in compat mode
c1477b0
to
1cf8b84
Compare
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
site/layouts/shortcodes/example.html
Outdated
@@ -37,7 +37,7 @@ | |||
|
|||
{{- if eq $show_preview true -}} | |||
<!-- OUDS mod: added `order-first` to handle the focus order --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the .border-default
class be added to this OUDS mod comment ?
|
||
### Sass mixins | ||
|
||
**No mixins are used to generate our background utilities**, but we do have some additional mixins for other situations where you'd like to create your own gradients. | ||
{{< bootstrap-compatibility false >}} | ||
**No mixins are used to generate our background utilities**, but we do have some additional mixins for other situations where you'd like to create your own gradients that shouldn't be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahah this small passive-agressive, you could but you shouldn't :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄 I was wondering if we should let it like Bootstrap and if people use them, we don't care, maybe I should do that 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think very few people will read this part, and even less will need to do this kind of thing :)
No problem to keep it like this
@@ -92,7 +92,7 @@ Or modify the default `border-color` of a component: | |||
Dangerous heading | |||
</div> | |||
|
|||
<div class="p-3 bg-info bg-opacity-10 border border-emphasized border-start-0 rounded-end"> | |||
<div class="p-3 text-bg-info bg-opacity-10 border border-emphasized border-start-0 rounded-end"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Humm I didn't not catch this while we were working on borders but shouldn't we use bootstrap classes since we are in the compatibility part ? border-brand-primary
and border-emphasized
are ouds classes. Not sure we were very consistent with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, good catch! I'll recheck the doc, but we should be using only Bootstrap classes inside bootstrap compatibility paragraphs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rechecked it, we only one class that was changed: .bg-primary
-> .bg-brand-primary
to have the same rendering than Bootstrap. Changed the rest.
@@ -66,10 +66,35 @@ In the following table we assume that the context is switching between light and | |||
| `.text-on-status-emphasized` | `.bg-status-accent-emphasized`, `.bg-status-positive-emphasized`, `.bg-status-warning-emphasized`, `.bg-status-info-emphasized` | — | | |||
| `.text-on-status-emphasized-alt` | `.bg-status-neutral-emphasized`, `.bg-status-negative-emphasized` | — | | |||
| `.text-on-status-muted` | `.bg-status-neutral-muted`, `.bg-status-accent-muted`, `.bg-status-positive-muted`, `.bg-status-negative-muted`, `.bg-status-warning-muted`, `.bg-status-info-muted` | — | | |||
| `.text-always-on-black` | `.bg-emphasized`, `.bg-always-black` | — | | |||
| `.text-always-on-white` | `.bg-brand-primary`, `.bg-status-accent-emphasized`, `.bg-status-positive-emphasized`, `.bg-status-warning-emphasized`, `.bg-status-info-emphasized`, `.bg-always-white` | — | | |||
| `.text-always-on-black` | `.bg-always-black` | — | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand well the documentation, those always-on
are meant to be used on some media content. Maybe bg-color
are not completely relevant here, but I would keep all dark background as acceptable with always-on-black and all light background as acceptable for the always-on-white. Like it was before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, after the small discussion we had, we definitely should have the previous markdown.
@@ -137,9 +137,9 @@ | |||
// OUDS mod: some values have been changed in the following `custom-color-mode` and `.btn-dropdown` is used instead of `.btn-secondary` | |||
// scss-docs-start custom-color-mode | |||
[data-bs-theme="blue"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have commented all the Blue bs-theme in custom-libraries markdown file. Do we still keep this content ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't decided yet if we should uncomment or delete the content, did we ? So I'm keeping this for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're probably right, we had an exchange about that bu we did not solve this case. So we just need to remember to be consistent later when we decide what to do.
@@ -173,7 +173,6 @@ | |||
@return $level; | |||
} | |||
|
|||
// TODO LM: see if we remove it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is it used ? I can't find any corresponding documentation :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not used but it might be great to have it, let's talk more about it
@@ -1,17 +1,17 @@ | |||
.themed-grid-col { | |||
padding-top: .75rem; | |||
padding-bottom: .75rem; | |||
background-color: var(--bs-secondary-bg); /* OUDS mod: instead of `rgba(112.520718, 44.062154, 249.437846, .15)` */ | |||
background-color: var(--bs-color-bg-secondary); /* OUDS mod: instead of `rgba(112.520718, 44.062154, 249.437846, .15)` */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -89,7 +89,7 @@ Here is a recap table of when to use which contextual theme. Prefer to use `ligh | |||
However, these 4 themes should be enough to deal with all of your use cases. If it's not the case, you are probably using them wrong. Since the implementation might be quite hard to understand, don't hesitate to contact us via our [GitHub discussions]({{< param repo >}}/discussions) with a reduced test case if you're having trouble implementing. | |||
|
|||
{{< callout info >}} | |||
We usually use **2 different layers to set the `background-color` and the theme**. In some rare case you might want to set it on one element. <!-- TODO LM: Fill in the blank and explain --> | |||
We use **2 different layers to set the `background-color` and the theme** when the theme doesn't inherit directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always inherit from a theme, so maybe we could rather say when the inherited theme does not match or something like that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I mean when you don't need to specify a [data-bs-theme]
but I'll try something clearer.
@@ -288,12 +288,20 @@ $utilities-colors: $theme-colors-rgb !default; | |||
$utilities-text: map-merge( | |||
$utilities-colors, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it useful to keep $utilities-colors
here since all ref are overwritten directly in the map. (I find it perturbing but I could understand to keep it for bootstrap migration questions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit confusing but my pov was:
- If people or a library override utilities colors on their own, they'll have more keys than what described in here so it might be good in some very rare cases
- Be closer to Bootstrap since we don't drop it for now
scss/helpers/_color-bg.scss
Outdated
@each $color, $value in $theme-colors { | ||
.text-bg-#{$color} { | ||
@if $enable-bootstrap-compatibility { | ||
@each $color, $value in map-remove($utilities-bg, "black", "white", "body") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we use $utilities-bg-colors
here like the one used in utilities ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I don't mind using the other one, it should be the same on our side, changing it
) | ||
) !default; | ||
$utilities-border-colors: map-loop($utilities-border, rgba-css-var, "$key", "border", "$value") !default; // OUDS mod | ||
$utilities-border-colors: $utilities-border !default; // OUDS mod: instead of `map-loop($utilities-border, rgba-css-var, "$key", "border", "$value")` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We end up with some useless local variables for border, bg and text. Each time we have a map, and just an affectation of this var to another var. It follows Bootstrap logic but it is harder to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's kind of related to the previous point, TBH I don't have a strong opinion on this, but I think the closer to Bootstrap we are, the easier it is to keep it maintained. So I'd keep them for now
- I think it's easier to understand to Bootstrap because we don't call the
map-loop(rgba-css-var)
which is hard to handle at the begining
text-align: var(--#{$prefix}body-text-align); | ||
|
||
/* rtl:remove */ | ||
letter-spacing: var(--#{$prefix}body-letter-spacing); // OUDS mod | ||
background-color: var(--#{$prefix}body-bg); // 2 | ||
background-color: var(--#{$prefix}color-bg-primary); // 2 // OUDS mod: instead of `var(--#{$prefix}body-bg)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand, you want to do that to make it possible to change it through body-bg
like its meant to be in bootstrap ? It would be taken into account even if not in compat mode
"black-50": var(--#{$prefix}color-content-muted), // Deprecated in Bootstrap // OUDS mod: instead of `rgba($black, .5)` | ||
"white-50": var(--#{$prefix}color-content-muted), // Deprecated in Bootstrap // OUDS mod: instead of `rgba($white, .5)` | ||
"body-secondary": var(--#{$prefix}color-content-muted), // OUDS mod: instead of `var(--#{$prefix}secondary-color)` | ||
"body-tertiary": var(--#{$prefix}color-content-muted), // OUDS mod: instead of `var(--#{$prefix}tertiary-color)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want a different value than secondary we may use disabled here. Maybe it too light ?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I've asked myself several times as well, my pov for this: when people use this variable on Bootstrap side https://getbootstrap.com/docs/5.3/customize/color-theme/, it's wrote: Use the tertiary color and bg options to have a body secondary less-emphasized rendering.
. To me it means that it should still be accessible to be read.
That said, if you prefer to have disabled in here, I don't really mind.
"body-secondary": rgba(var(--#{$prefix}secondary-bg-rgb), var(--#{$prefix}bg-opacity)), | ||
"body-tertiary": rgba(var(--#{$prefix}tertiary-bg-rgb), var(--#{$prefix}bg-opacity)), | ||
"body-secondary": var(--#{$prefix}color-bg-secondary), // OUDS mod: instead of `rgba(var(--#{$prefix}secondary-bg-rgb), var(--#{$prefix}bg-opacity))` | ||
"body-tertiary": var(--#{$prefix}color-bg-secondary), // OUDS mod: instead of `rgba(var(--#{$prefix}tertiary-bg-rgb), var(--#{$prefix}bg-opacity))` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be color-bg-tertiary
if we want a difference with secondary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, it might look weird on Bootstrap compat but I don't really mind to change it.
|
||
Sass cannot programmatically generate variables, so we manually created variables for every tint and shade ourselves. We then map all the created variables to one of our raw color tokens that are coming from the design directly. | ||
|
||
<h3>Example</h3> | ||
<h4>Example</h4> | ||
|
||
Here's how you should use these in your Sass: | ||
|
||
```scss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an issue with the rendering of this code sample. But If we change the parameter of bootstrap-compatibility short code we will have an issue elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good catch, I changed it in the last commit.
@@ -218,4 +228,4 @@ $utilities: map-merge( | |||
@import "@ouds/web/scss/utilities/api"; | |||
``` | |||
|
|||
This will generate new `.text-{color}-{level}` utilities for every color and level. You can do the same for any other utility and property as well. --> | |||
This will generate new `.text-{color}-{level}` utilities for every color and level. You can do the same for any other utility and property as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really asking myself about this documentation page. The content is consistent and fit with OUDS. But good luck to the guy that will need to use any of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's more a page to understand how OUDS work if people to get dive into the mechanism, I still need to add color-theme.md
which is supposed to be related to this one with this one referencing the raw variables and the color theme page the semantic variables.
Note: Please transform
- [ ]
into- (NA)
in the description when things are not applicableRelated issues
NA
Description
Remaining tasks and questions
Tasks:
Done list
The following was done in the PR:
TODO LM
occurrences by solving them or postponing for future us.OUDS mod
_colored-links.scss
border-opacity
,text-opacity
andbg-opacity
To be done after the PR is merged
OUDS mod
Motivation & Context
Types of change
Live previews