Conversation
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
# Conflicts: # site/data/sidebar.yml
louismaximepiton
left a comment
There was a problem hiding this comment.
There's probably a link to uncoment in the button page.
All oran.ge/icons should probably be replaced by {{< param icons >}}.
icon-link.md page should be dropped since it's no more in sidebar.yml.
Helpers > Icon entry should be removed from the sidebar.yml
| The following color utilities and CSS variables are intended for use only with functional icons, not with text, and icons should not be used on their own, but only alongside text, as functional colors do not provide sufficient contrast to ensure readability. | ||
|
|
||
| {{< example >}} | ||
| <div class="d-flex justify-content-center gap-md-tall"> |
There was a problem hiding this comment.
As you raised earlier, it's very hard to understand design choices about not making them accessible.
There was a problem hiding this comment.
I really don't understand the why of these choices of non-accessible icon colors, what can we do ?
|
|
||
| ### Other icons | ||
|
|
||
| Here are some examples of other [color utilities]({{< docsref "utilities/colors#colors" >}}) and CSS variables used with icons. |
There was a problem hiding this comment.
Should we add a table like the one above ?
There was a problem hiding this comment.
I could, but I don't have the context of usage...
There was a problem hiding this comment.
I feel this could be an improvement for the future, let's not close it but feel free not to fill it in this PR.
| The following color utilities and CSS variables are intended for use only with functional icons, not with text, and icons should not be used on their own, but only alongside text, as functional colors do not provide sufficient contrast to ensure readability. | ||
|
|
||
| {{< example >}} | ||
| <div class="d-flex justify-content-center gap-md-tall"> |
There was a problem hiding this comment.
I really don't understand the why of these choices of non-accessible icon colors, what can we do ?
| - CSS background images are intended to be decorative | ||
|
|
||
| #### Informative/meaningful icons | ||
| If the icon is **meaningful**, e.g. only content of a button, you have to provide an appropriate alternative text: for example, the description of the icon or the description of the action triggered. The best way to do this is to keep the icon hidden to assistive technologies (see above) and add a visually hidden label (which will be perceived by assistive technologies) by using the `.visually-hidden` class. For external images, you can also fill the `alt` attribute directly. |
There was a problem hiding this comment.
My adding for these last sentences:
The best way to do this is to keep the icon hidden to assistive technologies (see above) and add a visually hidden label (which will be perceived by assistive technologies) by using the .visually-hidden class. For external images, you can also fill in the alt attribute directly. Instead of these two techniques, you can also use an aria-label in the button tag to provide an accessible name.
There was a problem hiding this comment.
Pull Request Overview
This PR restores and updates documentation for icons in the project by consolidating icon usage instructions and references across several pages. Key changes include:
- Updating the buttons, links, typography, and text documentation to reference the new icon size utilities and SVG sprite guidelines.
- Adjusting the sidebar to remove draft flags for icon-related pages.
- Removing the obsolete icon-link helper documentation.
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| site/content/docs/0.2/components/buttons.md | Updated icon usage instructions in buttons with an informative callout. |
| site/content/docs/0.2/components/links.md | Revised icon usage guidance in links and improved accessibility recommendations. |
| site/content/docs/0.2/utilities/text.md | Updated reference from icon helper to icon size utilities. |
| site/content/docs/0.2/content/typography.md | Consistent update of icon helper reference to icon size utilities. |
| site/data/sidebar.yml | Removed draft flags to make the Icons documentation live. |
| site/content/docs/0.2/utilities/colors.md | Simplified icon-related color utility guidance by linking to the proper icons documentation. |
| site/content/docs/0.2/helpers/icon-link.md | Removed obsolete icon-link helper documentation. |
Files not reviewed (2)
- site/assets/scss/_component-examples.scss: Language not supported
- site/content/docs/0.2/examples/grid-system/index.html: Language not supported
Comments suppressed due to low confidence (2)
site/content/docs/0.2/helpers/icon-link.md:1
- The file icon-link.md has been completely removed. Please confirm that all references to the icon-link helper have been updated or removed to avoid broken documentation links.
----
site/data/sidebar.yml:217
- Ensure that the removal of the 'draft: true' flag for the Icons page is intentional, so the Icons documentation appears correctly in the sidebar.
- draft: true
|
|
||
| ### Other icons | ||
|
|
||
| Here are some examples of other [color utilities]({{< docsref "utilities/colors#colors" >}}) and CSS variables used with icons. |
There was a problem hiding this comment.
I feel this could be an improvement for the future, let's not close it but feel free not to fill it in this PR.
| <button type="button" class="btn btn-icon btn-default" aria-label="Open settings"> | ||
| <span class="icon si si-settings" aria-hidden="true"></span> | ||
| </button> | ||
| <img src="/docs/{{< param docs_version >}}/assets/img/heart-recommend.svg" alt="Favorite" width="32" height="32"> |
There was a problem hiding this comment.
Is that normal to have it here ? It feels weird to have it, I couldn't spot it in dark mode at the first glance.
There was a problem hiding this comment.
@louismaximepiton, I find it logical to have an example with an image since we talk about the alt attribute. Maybe we should use another icon color that could work on white and dark background ?
…nd-icons-page # Conflicts: # site/content/docs/0.3/helpers/icon-link.md # site/content/docs/0.3/helpers/icon.md # site/static/docs/0.3/assets/img/heart-recommend.svg
louismaximepiton
left a comment
There was a problem hiding this comment.
2 more small changes and we're good to go
| <symbol fill="currentColor" viewBox="0 0 1000 1000" id="tick-confirmation"> | ||
| <path d="M500 75C265.279 75 75 265.279 75 500s190.279 425 425 425 425-190.279 425-425S734.721 75 500 75Zm222.051 264.988-254.642 320-.007-.006a39.635 39.635 0 0 1-62.125 0l-.006.005-127.321-160 .006-.006a40.142 40.142 0 0 1 2.928-53.267l15.915-16a39.612 39.612 0 0 1 54.022-2.077l.006-.008L428.382 515 663.77 285.818l.006.008A39.734 39.734 0 0 1 730.77 315a39.952 39.952 0 0 1-8.726 24.981Z" style="fill-rule:evenodd"/> | ||
| </symbol> | ||
| <symbol id="vector" viewBox="0 0 46 48" fill="currentColor"> |
There was a problem hiding this comment.
| <symbol id="vector" viewBox="0 0 46 48" fill="currentColor"> | |
| <symbol fill="currentColor" viewBox="0 0 46 48" id="vector"> |
# Conflicts: # site/content/docs/0.3/components/buttons.md # site/content/docs/0.3/helpers/icon.md
Related issues
#2887
Description
Motivation & Context
During beta user tests, we saw than icons documentation is needed. Now that we know that we will still use the Solaris icons library, we can restore the page.
Types of change
Live previews