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.

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] Docs review after first OUDS version (including opacity tokens) #2713

Merged
merged 21 commits into from
Sep 23, 2024

Conversation

hannahiss
Copy link
Member

@hannahiss hannahiss commented Sep 2, 2024

Note: Please transform - [ ] into - (NA) in the description when things are not applicable

Related issues

#2589

Description

Docs review after first PRs from 2589 issue: suggestions for improving documentation or reporting problems, review opacity tokens.
Changes:

Other questions:

Motivation & Context

Review all available documentation at the moment

Types of change

  • Bug fix (non-breaking which fixes an issue) =>Docs update suggestions

Live previews

@hannahiss hannahiss changed the title [OUDS] Docs review after [OUDS] Docs review after first version Sep 2, 2024
@hannahiss hannahiss changed the base branch from main to ouds/main September 2, 2024 16:43
Copy link

netlify bot commented Sep 2, 2024

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit fd14bdf
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/66f11b2241968c00082d3c74
😎 Deploy Preview https://deploy-preview-2713--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.

@hannahiss hannahiss changed the title [OUDS] Docs review after first version [OUDS] Docs review after first OUDS version Sep 3, 2024
@hannahiss hannahiss changed the title [OUDS] Docs review after first OUDS version [OUDS] Docs review after first OUDS version (including opacity tokens) Sep 4, 2024
@hannahiss hannahiss marked this pull request as ready for review September 4, 2024 08:02
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Docs review after first PRs from 2589 issue: suggestions for improving documentation or reporting problems, review opacity tokens.
Changes:

Answered as a comment.

Fine with it.

Its definitely missing out there.

I think we should because the mixin provides it. And it's not meant to be changed so we can keep it as-is for now imo.

Fine with it.

Yep, feel free to open a PR upstream.

Answered as a comment.

Fine with it.

Other questions:

Good catch, I think it was done before we had any utility, but now it seems that it is readable

I think it definitely lack some explanation somewhere, feel free to try an helping text, maybe a new page inside Extend part of the doc ? Or maybe as you said only a paragraph that we can link upon the documentation.

No, there no link, the z-index tokens are more for components things. These are only helpers that don't appear in the tokens because they are very web related.

Yeah why not, TBH if you think that there are some improvements to do, feel free to change the link so we can review it 😄

site/content/docs/0.0/getting-started/accessibility.md Outdated Show resolved Hide resolved
site/content/docs/0.0/migration.md Outdated Show resolved Hide resolved
site/content/docs/0.0/utilities/display.md Outdated Show resolved Hide resolved
site/content/docs/0.0/about/license.md Outdated Show resolved Hide resolved
site/content/docs/0.0/customize/options.md Show resolved Hide resolved
site/content/docs/0.0/customize/options.md Outdated Show resolved Hide resolved
@julien-deramond julien-deramond added this to the OUDS milestone Sep 19, 2024
# Conflicts:
#	site/layouts/shortcodes/bootstrap-compatibility.html
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Good job @hannahiss! The doc is definitely enhanced and clearer this way, thanks for this work 💪
Here are a few comments for details here and there.

site/content/docs/0.0/customize/custom-libraries.md Outdated Show resolved Hide resolved
site/content/docs/0.0/extend/approach.md Outdated Show resolved Hide resolved
site/content/docs/0.0/extend/approach.md Outdated Show resolved Hide resolved
site/content/docs/0.0/migration.md Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

In this file and the Boosted migration, we often say:

"[...] only available when $enable-bootstrap-compatibility is on"

Could be a link to the new content at /docs/extend/approach/#bootstrap-compatibility

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the modification @hannahiss
Maybe it should be done in the "migration-from-boosted.md" too when we say "You can still have them using $enable-bootstrap-compatibility.".

Copy link
Member Author

Choose a reason for hiding this comment

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

done. Maybe we should also move the paragraph "from now on, ..." up in the text, so everyone sees it at the beginning of the migration guide

Copy link
Member

@julien-deramond julien-deramond Sep 23, 2024

Choose a reason for hiding this comment

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

In this file (migration.md), it's a migration version per version. When we'll release the first real versions, we'll change the content of this file, and yeah, we should put it at the beginning of the migration guide.
For now, IMO, it's OK as is.

@julien-deramond julien-deramond merged commit 8cd7c1f into ouds/main Sep 23, 2024
18 checks passed
@julien-deramond julien-deramond deleted the ouds/main-his-2589-review branch September 23, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants