-
Notifications
You must be signed in to change notification settings - Fork 54
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
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.
Docs review after first PRs from 2589 issue: suggestions for improving documentation or reporting problems, review opacity tokens.
Changes:
- in https://deploy-preview-2713--boosted.netlify.app/docs/getting-started/accessibility/ the link name should be WCAG 2.2 and not 2.1
Answered as a comment.
- in https://deploy-preview-2713--boosted.netlify.app/docs/getting-started/contents/ and https://deploy-preview-2713--boosted.netlify.app/docs/customize/custom-libraries/: additional information in the formulation
Fine with it.
- in https://deploy-preview-2713--boosted.netlify.app/docs/customize/options/: is
$enable-cssgrid
mentionned in https://deploy-preview-2713--boosted.netlify.app/docs/layout/css-grid/ not missing? (from Bootstrap)
Its definitely missing out there.
- in https://deploy-preview-2713--boosted.netlify.app/docs/utilities/api/: do we need to keep RTL option and paragraph about RTL since RTL is not yet implemented/documented?
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.
- in https://deploy-preview-2713--boosted.netlify.app/docs/utilities/display/: removed the word "semantic" as it may be confusing for the reader
Fine with it.
- in https://deploy-preview-2713--boosted.netlify.app/docs/utilities/flex/#align-content, "heads up" is not in a callout as usual (from Bootstrap)
Yep, feel free to open a PR upstream.
- in https://deploy-preview-2713--boosted.netlify.app/docs/about/license/: there were two different links with the same accessible name which is an accessibility issue
Answered as a comment.
- in https://deploy-preview-2713--boosted.netlify.app/docs/migration/#pre-compiled-versions I proposed another wording but I'm not totally satisfied either (see question below)
Fine with it.
Other questions:
- in https://deploy-preview-2713--boosted.netlify.app/docs/extend/approach/: I can't see why the paragraph Utilities has not been uncommented?
Good catch, I think it was done before we had any utility, but now it seems that it is readable
- between https://deploy-preview-2713--boosted.netlify.app/docs/customize/options/ and https://deploy-preview-2713--boosted.netlify.app/docs/utilities/opacity/, I feel like some more explanations are missing somewhere about
$enable-bootstrap-compatibility
, in particular about the philosophy of having our own tokens, that give their names and values to utilities that can be different from those from Bootstrap. Then, in the opacity utilities page, we could link this page/paragraph of the documentation. Could be in https://deploy-preview-2713--boosted.netlify.app/docs/extend/approach? I found later that a small explanation is there https://deploy-preview-2713--boosted.netlify.app/docs/migration/#pre-compiled-versions (but I don't totally agree with the wording, see above)
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.
- question about https://deploy-preview-2713--boosted.netlify.app/docs/utilities/z-index/: these utilities so have no link with our elevation tokens?:
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.
- in https://deploy-preview-2713--boosted.netlify.app/docs/customize/custom-libraries/: About the example in Stackblitz (https://stackblitz.com/edit/github-j5teen) I was wondering if we should comment or remove the unused values in the tokens
_custom-raw.scss
like we do in OUDS-web?
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 😄
Co-authored-by: Louis-Maxime Piton <[email protected]>
# Conflicts: # site/layouts/shortcodes/bootstrap-compatibility.html
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.
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.
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 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
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.
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
.".
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.
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
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 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.
Note: Please transform
- [ ]
into- (NA)
in the description when things are not applicableRelated issues
#2589
Description
Docs review after first PRs from 2589 issue: suggestions for improving documentation or reporting problems, review opacity tokens.
Changes:
$enable-cssgrid
mentionned in https://deploy-preview-2713--boosted.netlify.app/docs/layout/css-grid/ not missing? (from Bootstrap)Other questions:
$enable-bootstrap-compatibility
, in particular about the philosophy of having our own tokens, that give their names and values to utilities that can be different from those from Bootstrap. Then, in the opacity utilities page, we could link this page/paragraph of the documentation. Could be in https://deploy-preview-2713--boosted.netlify.app/docs/extend/approach? I found later that a small explanation is there https://deploy-preview-2713--boosted.netlify.app/docs/migration/#pre-compiled-versions (but I don't totally agree with the wording, see above)_custom-raw.scss
like we do in OUDS-web?Motivation & Context
Review all available documentation at the moment
Types of change
Live previews