-
Notifications
You must be signed in to change notification settings - Fork 56
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 "Grid" tokens, utilities, examples and documentation #2744
base: ouds/main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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.
First batch of comments.
A fake example should be removed somewhere.
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.
Second batch, missing the 4 documentation files, 1 example and migration guides
<div class="container-fluid max-width-specific-tools mb-3 pt-3"> | ||
<p class="text-center">Class <code class="text-black">.container-xs</code></p> | ||
</div> | ||
<div class="container-xs mb-3 bg-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.
Do we show .container-{breakpoint}
in here ? Since they shouldn't be used, maybe only .container
(to show the difference of behavior with the others), .container-fluid
, .container-fluid.site-width-*
should be shown in here ? If you accept this, we might add additional CSS to make the body 100% height.
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 the user (developer) wouldn't understand why in grid example we have all containers and not there. Maybe I can change the order and add a disclaimer... Maybe a disclaimer too on grid example to be consistent... Let's give it a try.
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'll review the migration guides once all the above are considered as done.
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.
Just pinging for few comments that can be resolved in my opinion. I'm reviewing the migration guides in the meantime. Great work, GG 🚀
scss/_containers.scss
Outdated
|
||
// OUDS mod | ||
// scss-docs-start containers-max | ||
.max-width-public-website { |
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, this naming doesn't mean anything, as everything is a public website, except "private" websites.
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.
Why do we display here things that can't be used? Are you planning to comment the incompatible ones in the end, ready to be uncommented for debugging purpose?
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.
This is something we have to discuss all together: in the grid example, all containers are presented... But I agree this can be confusing...
--- | ||
|
||
<main> | ||
<div class="container-fluid max-width-specific-tools"> |
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 why we use .max-width-specific-tools
and not .max-width-public-website
. Same thing for the OUDS Web documentation.
scss/_containers.scss
Outdated
} | ||
} | ||
|
||
.max-width-specific-tools { |
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 the .mw-100
utility, shouldn't we name these grid utilities .mw-*
too?
// scss-docs-end grid-gutters | ||
|
||
// scss-docs-start gutters | ||
$gutters: ( |
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.
Note: this must be mentioned in the migration guide.
site/content/docs/0.0/migration.md
Outdated
- <details class="mb-2"> | ||
<summary><span class="badge text-bg-danger">Breaking</span> Dropped Sass variables:</summary> | ||
<ul> | ||
<li><code>$grid-gutter-breakpoint</code></li> |
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 dropped from the Boosted point of view. So this part should rather be in the other migration guide, and not mentioned here as the grids were not supposed to exist before.
@@ -487,12 +503,17 @@ This will now generate responsive variations of `.border` and `.border-0` for ea | |||
.border { ... } | |||
.border-0 { ... } | |||
|
|||
@media (min-width: 390px) { | |||
.border-xs { ... } |
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.
With $enable-bootstrap-compatibility
, the .border-xs
, .border-md
, etc. are not generated. Only the *-0
versions. This is probably due to the modifications we've done for OUDS Web on the border side.
So either we fix this behavior for the borders, either we change here the sentence explaining what it's going to do + the examples of generated code.
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.
Give it a try with border-ouds
abbr: "" | ||
name: X-Small | ||
name: 2X-Small |
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.
In https://unified-design-system.orange.com/472794e18/p/26a48e-grid/t/e076cd8936, the names are rather "2xs", "xs", etc. Maybe we can use the same. Otherwise we can keep Bootstrap naming, "Extra extra small", etc.
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.
At first I wanted to keep Bootstrap names, but "Extra extra extra large" is really clumsy (lourdingue 😃 ).
Anyway, it has to be the same everywhere (see above)
|
||
### Offsetting columns | ||
|
||
You can offset grid columns in two ways: our responsive `.offset-` grid classes and our [margin utilities]({{< docsref "/utilities/spacing" >}}). Grid classes are sized to match columns while margins are more useful for quick layouts where the width of the offset is variable. |
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.
This sentence must be updated to remove the "margin utilities" mention (and link) since the dimension tokens and utilities don't exist yet.
</div> | ||
{{< /example >}} | ||
|
||
#### Margin 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.
Margin utilities don't exist yet. This section must be commented.
{{< bs-table "table" >}} | ||
| Breakpoint | Class infix | Dimensions | | ||
| --- | --- | --- | | ||
| 2X-small | <em>None</em> |<390px | |
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 should probably use the name from https://unified-design-system.orange.com/472794e18/p/26a48e-grid/t/e076cd8936 or Bootstrap
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 sure, because we would have almost exactly the same content in the first and second column? This way seems a little bit more explicit maybe?
|
||
// Usage | ||
|
||
// Example: Hide starting at `min-width: 0`, and then show at the `sm` breakpoint |
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.
They use sm
in Bootstrap because it's the first one. We could use xs
instead here.
|
||
// OUDS mod | ||
// scss-docs-start containers-max | ||
.container-max-width { |
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 to see if we shouldn't keep the name used by ob1, which is .site-width
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 need to find a name that fits well with the framework naming.
## Fluid containers | ||
|
||
Use `.container-fluid` for a full width container with minimum margins, spanning almost the entire width of the viewport. | ||
|
||
**`.container-fluid` is the container recommended for Orange sites in order to be compliant with the Orange brand.** | ||
|
||
```html | ||
<div class="container-fluid"> | ||
... | ||
</div> | ||
``` | ||
|
||
### Default max width | ||
|
||
This container is the default one to use, by default associated with the class `.container-max-width`. After breakpoint `2xl`, this class: | ||
- limits the container width to 1680px (including margins), | ||
- limits container margin to 80px each side, | ||
- limits gutters width to 32px. | ||
|
||
It is defined as follows: | ||
|
||
{{< scss-docs name="containers-max" file="scss/_containers.scss" >}} | ||
|
||
Comparison between `.container-fluid` and `.container-fluid` width `.container-max-width` can be seen in the following table: | ||
|
||
{{< bs-table "table" >}} | ||
| | X-Large<div class="fw-normal">≥1320px</div> | 2X-Large<div class="fw-normal">≥1640px</div> | 3X-Large<div class="fw-normal">≥1880px</div> | | ||
| --- | --- | --- | --- | --- | | ||
| `.container-fluid` | <ul><li>Width `100% - (2 * 56px)`</li><li>Margin `2 * 56px`</li><li>Gutter `32px`</li></ul> | <ul><li>Width `100% - (2 * 80px)`</li><li>Margin `2 * 80px`</li><li>Gutter `32px`</li></ul> | <ul><li>Width `100% - (2 * 112px)`</li><li>Margin `2 * 112px`</li><li>Gutter `40px`</li></ul> | | ||
| `.container-fluid`<br>with `.container-max-width` | <ul><li>Width `100% - (2 * 56px)`</li><li>Margin `2 * 56px`</li><li>Gutter `32px`</li></ul> | <ul><li>Width `100% - (2 * 80px)`</li><li><b>Max-width `1520px`</b></li><li>Margin `2 * 80px`</li><li>Gutter `32px`</li></ul> | <ul><li><b>Width `1520px`</b></li><li><b>Margin `2 * 80px`</b></li><li><b>Gutter `32px`</b></li></ul> | | ||
{{< /bs-table >}} | ||
|
||
### Custom max width | ||
|
||
You can set a custom maximum width for the fluid container by overloading `$ouds-grid-container-max-width` before importing our OUDS scss files. | ||
|
||
This will affect all containers that have `.container-max-width` and will make sure that your layout does not exceed this value. | ||
When going up the breakpoint above `$ouds-grid-container-max-width` value, the container margins and gutter values are kept as they are. | ||
|
||
## CSS | ||
|
||
### Sass variables | ||
|
||
As shown above, OUDS Web generates a series of predefined container classes to help you build the layouts you desire. You may customize these predefined container classes by modifying the Sass map (found in `_variables.scss`) that powers them: | ||
|
||
{{< scss-docs name="container-max-widths" file="scss/_variables.scss" >}} | ||
|
||
For more information and examples on how to modify our Sass maps and variables, please refer to [the Sass section of the Grid documentation]({{< docsref "/layout/grid#css" >}}). | ||
|
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.
Maybe this part should be changed if we want to stick to OB1's way to do it (having .site-width
on a parent of .container-fluid
). To see what we do for the start of this page: if we keep it like this, it starts with all the containers developers shouldn't use, it's confusing...
- `xxl` has been renamed `2xl`. | ||
- `2xs` and `3xl` have been added. | ||
- The breakpoints values have changed. Please refer to the [breakpoints' documentation]({{< docsref "/layout/breakpoints/" >}}). | ||
- `.col-{breakpoint}-{number}`, `.g-col-{}` |
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.
Don't know where this line is coming from?
- `.col-{breakpoint}-{number}`, `.g-col-{}` | ||
- The default container to use is now `.container-fluid` associated to `.container-max-width` instead of `.container-xxl`. Please refer to the [fluid containers' documentation]({{< docsref "/layout/containers/#fluid-containers" >}}). Thus, other containers have been removed: `.container` and `.container-{breakpoint}`. | ||
- Default gutter inside grid have been changed to have a fully responsive behavior. It should be a transparent change for you. | ||
- All gutter utilities have been changed. `.g{-breakpoint}-{value}`, `.gx{-breakpoint}-{value}` and `.gy{-breakpoint}-{value}` which value is inside `0|1|2|3|4|5`. Gutter utilities values now use `none|smash|shortest|shorter|short|medium|tall|taller|tallest|spacious|huge|jumbo`. Proportional equivalence between `0` → `none`, `1` → `shortest`, `2` → `shorter`, `3` → `medium`, `4` → `tall`, `5` → `spacious`. |
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.
To move inside dimension PR #2754 ?
Co-authored-by: Julien Déramond <julien.deramond@orange.com> Co-authored-by: Louis-Maxime Piton <louismaxime.piton@orange.com>
373e453
to
f2a2a25
Compare
60b7093
to
70308ad
Compare
70308ad
to
33c6ba1
Compare
Note: Please transform
- [ ]
into- (NA)
in the description when things are not applicableRelated issues
Closes #2609
Description
.container-fluid .max-width-specific-tools
as container for the documentation website_assert-ascending
to accept two identical values that follow each otherNot included:
Motivation & Context
Types of change
Live previews
Uncommented pages:
Modified pages:
New pages: