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.

Sign up for GitHub

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

Jump to bottom

[OUDS] Add "Dimension" tokens, utilities and documentation #2754

Open
wants to merge 10 commits into
base: ouds/main
Choose a base branch
from
Open

[OUDS] Add "Dimension" tokens, utilities and documentation #2754

wants to merge 10 commits into from

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Oct 7, 2024 •
edited by julien-deramond
Loading

Related issues

Closes #2605.

Description

Remaining tasks and questions

Questions:

  • Should we introduce an easy way to get all utilities in px and in rem. I mean not only a function px-to-rem but an option like $enable-rem-utilities that make a dupe of each utility but in rem? IMO, we should but it will probably make the bundle size raise a lot. Not done yet.
  • What scenario to follow inside the _utilities.scss for utilities priority? IMO we should stick to the one proposed for the doc implementation.
  • What to do with the $spacer and $spacers variables? IMO, $spacer should take the value $ouds-dimension-base * 5. Not done yet. It's the base of many calculations inside Boosted/Bootstrap. Do we replace them with our $ouds-dimension-base and $ouds-dimension-space-fixed? IMO, we should keep the Bootstrap ones.
  • Do we keep the same size utilities .w-100, etc.. ? IMO, we should since the tokens don't look like they are done for it.

Done list

The following was done in the PR:

  • Revert [OUDS] feat: Add "Elevation" tokens, utilities and documentation - new version #2741 changes about _ouds-maps.scss and rather introduce tokens/_composite.scss. Change all the related files.
  • Introduce new Sass maps that contain all the fixed and scaled values for utilities.
  • Introduce new CSS variables in the _root.scss file for the scaled utilities.
  • Introduce all the new utilities (fixed and scaled).
  • Introduce all new design tokens in tokens/_raw.scss and in tokens/_semantic.scss
  • Checked all the new provided options.
  • Quite complete migration guides.
  • Introduce the documentation of sizing.md and spacing.md utilities.
  • Introducing the tests of the spacings.

To be done after the PR is merged

  • .

Motivation & Context

Types of change

  • New feature (non-breaking change which adds functionality)

Live previews

@louismaximepiton louismaximepiton added feature docs Improvements or additions to documentation css labels Oct 7, 2024
@louismaximepiton louismaximepiton added this to the OUDS milestone Oct 7, 2024
@hannahiss hannahiss mentioned this pull request Oct 9, 2024
Merged
@louismaximepiton louismaximepiton force-pushed the ouds/main-lmp-tokens-spacing branch 2 times, most recently from a70db7f to eec9a1b Compare October 23, 2024 10:10
@louismaximepiton louismaximepiton mentioned this pull request Oct 23, 2024
Draft
17 tasks
@louismaximepiton louismaximepiton force-pushed the ouds/main-lmp-tokens-spacing branch 2 times, most recently from 2f78431 to a378e58 Compare October 23, 2024 13:49
Base automatically changed from ouds/main-his-tokens-grid to ouds/main October 24, 2024 12:55
@netlify Netlify
Copy link

netlify bot commented Oct 24, 2024 •
edited
Loading

✅ Deploy Preview for boosted ready!

Name Link
🔨 Latest commit 6759b74
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/6723375b5e7c6400085851f0
😎 Deploy Preview https://deploy-preview-2754--boosted.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@louismaximepiton louismaximepiton force-pushed the ouds/main-lmp-tokens-spacing branch 2 times, most recently from 33dc6ce to fdab9a8 Compare October 25, 2024 08:02
@louismaximepiton louismaximepiton marked this pull request as ready for review October 25, 2024 10:03
julien-deramond
julien-deramond reviewed Oct 29, 2024
View reviewed changes
site/content/docs/0.0/utilities/z-index.md
<div class="z-1 position-absolute p-5"><span>z-1</span></div>
<div class="z-0 position-absolute p-5"><span>z-0</span></div>
<div class="z-n1 position-absolute p-5"><span>z-n1</span></div>
<div class="z-3 position-absolute p-spacious"><span>z-3</span></div>
Copy link
Member

Choose a reason for hiding this comment

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

Please don't take this comment for granted and don't modify the PR. This is a complex one to review, so I'll put some comments here and there with my 2 cents. Hopefully, in the end, with your rules, some comments, and the designers inputs, we might create a cool mapping table for usage, but also for migrations.

Here, the previous value is 60px. So I would rather use as a real project "huge" (56px) or "jumbo" (64px) to have the same rendering.
The previous rule is that the values doubled between 0,1, 2, 3, 4, 5 → 0, 5px, 10px, 20px, 30px, 60px.
If we keep this rule I would rather use for equivalent usages and easier migration none, shortest, shorter, medium, taller, jumbo instead of the current none, shortest, shorter, medium, tall, spacious.

julien-deramond
julien-deramond reviewed Oct 29, 2024
View reviewed changes
site/content/docs/0.0/utilities/overflow.md
<code>.overflow-y-hidden</code> example on an element with set width and height dimensions.
</div>
<div class="overflow-y-visible p-3 mb-3 mb-md-0 me-md-3 w-100 border" style="max-width: 200px; max-height: 100px;">
<div class="overflow-y-visible p-medium mb-medium mb-md-none me-md-medium w-100 border" style="max-width: 200px; max-height: 100px;">
Copy link
Member

Choose a reason for hiding this comment

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

There's an issue with this example, as the text is not long-enough to show the feature. It's mainly because we go from 20px padding to 16px padding. Maybe we should use tall instead in the entire page?

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for #2754 (comment) final values as it could be fixed automatically

julien-deramond
julien-deramond reviewed Oct 29, 2024
View reviewed changes
site/layouts/_default/docs.html
@@ -1,6 +1,6 @@
{{ define "body_override" }}<body{{ if (eq .Page.Params.toc true) }} data-bs-spy="scroll" data-bs-target="#TableOfContents"{{ end }}>{{ end }}
{{ define "main" }}
<div class="container-fluid container-max-width bd-gutter mt-3 my-md-4 bd-layout">
<div class="container-fluid container-max-width bd-gutter mt-medium my-md-tall bd-layout">
Copy link
Member

@julien-deramond julien-deramond Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This layout sounds a little bit too compact. It was pretty tight already in Boosted docs. Let's check with the designers.

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for #2754 (comment) final values. Could be fixed automatically.

@julien-deramond
Copy link
Member

Should we introduce an easy way to get all utilities in px and in rem. I mean not only a function px-to-rem but an option like $enable-rem-utilities that make a dupe of each utility but in rem? IMO, we should but it will probably make the bundle size raise a lot. Not done yet.

In most cases, while building a page with the layouts, it makes more sense most of the time to use pixels for paddings and margins as we don't want them to change while zooming out. In the components, we might use both depending on the use cases, but we're not supposed to use utilities to build components.
So I'd say, we should be fine with utilities in pixels by default, and having them in rem might be something done on the projects' side. We could start without this $enable-rem-utilities and wait for projects' feedback.

@julien-deramond
Copy link
Member

What to do with the $spacer and $spacers variables? IMO, $spacer should take the value $ouds-dimension-base * 5. Not done yet. It's the base of many calculations inside Boosted/Bootstrap. Do we replace them with our $ouds-dimension-base and $ouds-dimension-space-fixed? IMO, we should keep the Bootstrap ones.

IMO, $spacer(s) should be defined from the $ouds-dimension-* Sass variables. And $spacers values should correspond to the migration matching table from Boosted. For instance, if we consider that p-1 must be p-shortest, then $spacers[1] should be defined with $ouds-space-fixed-shortest, etc.
Then, for all OUDS Web utilities, components, layouts, etc. we should use values depending on $ouds-space or $ouds-dimension semantic variables. In the end, $spacer(s) would be used only to define the Bootstrap compatibility utilities, and be ther for plugins and libraries depending on that.

@julien-deramond
Copy link
Member

Do we keep the same size utilities .w-100, etc.. ? IMO, we should since the tokens don't look like they are done for it.

It concerns only icons and typos, so let's postpone it.

julien-deramond
julien-deramond reviewed Oct 30, 2024
View reviewed changes
site/content/docs/0.0/layout/gutters.md

- **Gutters start at `2px` and go up to `64px` wide.** This allows us to match our grid to the [padding and margin spacers]({{< docsref "/utilities/spacing" >}}) scale.
- **Gutter utilities start at `2px` and go up to `64px` wide.** This allows us to match our grid to the [padding and margin spacers]({{< docsref "/utilities/spacing" >}}) scale.
Copy link
Member

Choose a reason for hiding this comment

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

Gutters was fine, wasn't it?

Suggested change
- **Gutter utilities start at `2px` and go up to `64px` wide.** This allows us to match our grid to the [padding and margin spacers]({{< docsref "/utilities/spacing" >}}) scale.
- **Gutters start at `2px` and go up to `64px` wide.** This allows us to match our grid to the [padding and margin spacers]({{< docsref "/utilities/spacing" >}}) scale.

julien-deramond
julien-deramond reviewed Oct 30, 2024
View reviewed changes
site/content/docs/0.0/layout/gutters.md
## Horizontal gutters

`.gx-*` classes can be used to control the horizontal gutter widths. The `.container` or `.container-fluid` parent may need to be adjusted if larger gutters are used too to avoid unwanted overflow, using a matching padding utility. For example, in the following example we've increased the padding with `.px-tall`. Note that in the previous example, we use a `short` gutter width, so there isn't a need for the `.overflow-hidden` wrapper class.
`.gx-*` classes can be used to control the horizontal gutter widths. The wrapper element may need to be adjusted if larger gutters are used too to avoid unwanted overflow, using a matching padding utility. For example, in the following example we've increased the padding with `.px-tall`.
Copy link
Member

Choose a reason for hiding this comment

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

You can say "The container", it works too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julien-deramond julien-deramond julien-deramond left review comments

Assignees
No one assigned
Labels
css docs Improvements or additions to documentation feature
Projects
[OUDS] Project
Status: In Progress
🟣 [Orange-Boosted-Bootstrap] PRs Board
Status: Need Dev Review
Milestone
OUDS
Development

Successfully merging this pull request may close these issues.
2 participants
@louismaximepiton @julien-deramond