-
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] Divider component #2932
base: ouds/main
Are you sure you want to change the base?
[OUDS] Divider component #2932
Conversation
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Pull Request Overview
This PR introduces a new Dividers component to provide horizontal and vertical rule helpers and updates the corresponding documentation and migration guides. Key changes include:
- Adding a new "Dividers" entry in the sidebar.
- Updating migration documentation to reference the new Dividers helper.
- Creating a new Dividers documentation file and updating horizontal rule examples in reboot.
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
site/data/sidebar.yml | Added a new "Dividers" entry to the sidebar navigation. |
site/content/docs/0.3/migration.md | Updated migration documentation with Dividers helper details. |
site/content/docs/0.3/migration-from-boosted.md | Updated migration-from-boosted documentation for dividers. |
site/content/docs/0.3/helpers/dividers.md | Added new documentation for Dividers helpers. |
site/content/docs/0.3/content/reboot.md | Revised horizontal rule examples and descriptions. |
Files not reviewed (2)
- scss/_variables.scss: Language not supported
- scss/helpers/_vr.scss: Language not supported
site/content/docs/0.3/migration.md
Outdated
- <span class="badge text-bg-status-warning-emphasized">Warning</span> The vertical rule documentation has been merged into is the new [Dividers helper]({{< docsref "/helpers/dividers" >}}) 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.
The phrase 'merged into is the new' is grammatically incorrect. Consider revising it to 'has been merged into the new' for clarity.
- <span class="badge text-bg-status-warning-emphasized">Warning</span> The vertical rule documentation has been merged into is the new [Dividers helper]({{< docsref "/helpers/dividers" >}}) documentation. | |
- <span class="badge text-bg-status-warning-emphasized">Warning</span> The vertical rule documentation has been merged into the new [Dividers helper]({{< docsref "/helpers/dividers" >}}) documentation. |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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.
Removed
- <span class="badge text-bg-status-warning-emphasized">Warning</span> The vertical rule documentation has been merged into is the new [Dividers helper]({{< docsref "/helpers/dividers" >}}) 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.
The phrase 'merged into is the new' is awkward. Consider updating it to 'has been merged into the new' to improve readability.
- <span class="badge text-bg-status-warning-emphasized">Warning</span> The vertical rule documentation has been merged into is the new [Dividers helper]({{< docsref "/helpers/dividers" >}}) documentation. | |
- <span class="badge text-bg-status-warning-emphasized">Warning</span> The vertical rule documentation has been merged into the new [Dividers helper]({{< docsref "/helpers/dividers" >}}) documentation. |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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.
Ok fixed
The `<hr>` color is automically adjusted according to the color modes or on colored backgrounds. | ||
|
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.
The word 'automically' appears to be a misspelling. It should be corrected to 'automatically'.
The `<hr>` color is automically adjusted according to the color modes or on colored backgrounds. | |
The `<hr>` color is automatically adjusted according to the color modes or on colored backgrounds. |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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.
Ok 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.
Maybe an <hr>
to change in:
- color-palette.md
- maybe a
.vr
example to uncomment in stacks.md - Some hr and vr might be broken in the doc: homepage for example
We can safely remove vertical-rule.md file.
scss/helpers/_vr.scss
Outdated
@@ -1,8 +1,12 @@ | |||
.vr { | |||
display: inline-block; | |||
align-self: stretch; | |||
width: $vr-border-width; | |||
// width: $vr-border-width; // OUDS mod: no 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.
// width: $vr-border-width; // OUDS mod: no width | |
// OUDS mod: no 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.
Ok done
scss/helpers/_vr.scss
Outdated
opacity: $hr-opacity; | ||
margin: 0 $ouds-space-fixed-medium; | ||
color: $ouds-color-border-default; | ||
// background-color: currentcolor; // OUDS mod: no background 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.
// background-color: currentcolor; // OUDS mod: no background color | |
// OUDS mod: no background 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.
Ok done
scss/helpers/_vr.scss
Outdated
// background-color: currentcolor; // OUDS mod: no background color | ||
border: 0; | ||
border-left: $vr-border-width solid $ouds-color-border-default; | ||
// opacity: $hr-opacity; // OUDS mod: no 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.
// opacity: $hr-opacity; // OUDS mod: no opacity | |
// OUDS mod: no 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.
Ok done
@@ -174,19 +174,33 @@ On colored backgrounds, links should have a different style. Visited links don't | |||
|
|||
## Horizontal rules | |||
|
|||
The `<hr>` element has been simplified. Similar to browser defaults, `<hr>`s are styled via `border-top`, have a default `opacity: .25`, and automatically inherit their `border-color` via `color`, including when `color` is set via the parent. They can be modified with text, border, and opacity utilities. | |||
The `<hr>` element has been simplified. Similar to browser defaults, `<hr>`s are styled via `border-top` and automatically inherit their `border-color` via `color`. They can be modified with border utilities. | |||
|
|||
<!-- TODO: Check once the dividers have been designed --> |
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 can safely remove 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.
Ok done
@@ -174,19 +174,33 @@ On colored backgrounds, links should have a different style. Visited links don't | |||
|
|||
## Horizontal rules | |||
|
|||
The `<hr>` element has been simplified. Similar to browser defaults, `<hr>`s are styled via `border-top`, have a default `opacity: .25`, and automatically inherit their `border-color` via `color`, including when `color` is set via the parent. They can be modified with text, border, and opacity utilities. | |||
The `<hr>` element has been simplified. Similar to browser defaults, `<hr>`s are styled via `border-top` and automatically inherit their `border-color` via `color`. They can be modified with border 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.
This sentence looks wrong for me, either the border color inherits from color and so color utilities can be used, or we don't inherit and the end is fine.
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.
Ok done
|
||
## Horizontal rules | ||
|
||
Horizontal rules should use the `<hr>` tag, when the separator has semantic meaning (i.e., when it represents a topic change or a significant transition in the content). If the divider is purely decorative, use a `<div>` styled with a `hr` class ensuring it is hidden from assistive technologies (e.g., `aria-hidden="true"`). |
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.
.hr
doesn't exist. maybe we should implement it ?
Horizontal rules should use the `<hr>` tag, when the separator has semantic meaning (i.e., when it represents a topic change or a significant transition in the content). If the divider is purely decorative, use a `<div>` styled with a `hr` class ensuring it is hidden from assistive technologies (e.g., `aria-hidden="true"`). | |
Horizontal rules should use the `<hr>` tag, when the separator has semantic meaning (i.e., when it represents a topic change or a significant transition in the content). If the divider is purely decorative, prefer using a border CSS property on a `<div>` or our [border utilities]({{< docsref "/utilities/border" >}}), or even ensure to hide it from assistive technologies (e.g., `aria-hidden="true"`). |
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.
Ok no .hr class
|
||
{{< example >}} | ||
<hr> | ||
<hr class="text-brand-primary"> |
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.
Are we allowed to use text utilities to modify a border looking helper ?
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.
Keep it in bootstrap compat
|
||
## Vertical rules | ||
|
||
Vertical rules are inspired by the `<hr>` element, allowing you to create vertical dividers in common layouts. They're styled just like `<hr>` elements. They have `min-height` of `1em`. |
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 1em
looks a bit arbitrary, but I can't figure out why it shouldn't be this 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.
Ok it comes from Boostrap
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'mwondering if we should add vertical-align
somehow to make sure people don't struggle too much in block context. Tried it using inline-blocks and the rendering was weird.
<hr> | ||
<hr class="border-emphasized border-medium"> | ||
<hr class="text-brand-primary border-thick"> |
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 question here, are we allowed to display this example ?
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.
Removed from reboot
245ed97
to
061c143
Compare
Related issues
Closes #2826
Description
Default
<hr>
styled in rebootNew divider helpers for horizontal and vertical rules
Motivation & Context
Types of change
Live previews
Checklist
Contribution
Accessibility
Design
Development
Documentation
Checklist (for Core Team only)