-
Notifications
You must be signed in to change notification settings - Fork 10
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
Update typography docs #2290
Update typography docs #2290
Conversation
🦋 Changeset detectedLatest commit: 9eabdf8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Change: 0 B Total Size: 95.4 kB ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2290 +/- ##
==========================================
+ Coverage 94.26% 95.20% +0.94%
==========================================
Files 253 253
Lines 30262 30262
Branches 1707 1856 +149
==========================================
+ Hits 28525 28810 +285
+ Misses 1732 1436 -296
- Partials 5 16 +11 see 47 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
b10c3ba
to
3a12f00
Compare
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-sckjurcquk.chromatic.com/ Chromatic results:
|
3a12f00
to
f40e868
Compare
@@ -3,24 +3,26 @@ export default { | |||
control: {type: "text"}, | |||
description: "Text to appear with the specified typography styles.", | |||
table: {type: {summary: "React.Node"}}, | |||
type: {required: false}, |
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.
Removing these addressed TypeScript errors! These aren't needed since these props are not being marked as required already!
@@ -0,0 +1,35 @@ | |||
import * as React from "react"; |
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.
Creating stories for each typography component so that they are more discoverable. These won't be included in Chromatic tests. All the new stories for typography components are based on the same changes!
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: NOT Published🤕 Oh noes!! We couldn't find any changesets in this PR (9edf54a). As a result, we did not publish an npm snapshot for you. |
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 working on all these improvements! I left some comments to convert the accessibility page to MDX.
|
||
import {BodyMonospace} from "@khanacademy/wonder-blocks-typography"; | ||
|
||
export default { |
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.
thought: It would be great if we could describe what's the general purpose for each typography component, but this is not something we document anywhere, probably something worth sharing in the WB cubby meeting this week.
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.
Great idea! That would be helpful for usage - I'm not sure myself when certain typography components should be used (ex: when should the different label sizes be used?) I added it to our agenda!
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.
suggestion: I think it would be better if this page would be created as a separate MDX file, so it can make better use of the Storybook docs feature.
You can see an example of how we use this here: https://github.com/Khan/wonder-blocks/blob/76967ed08c63c4bd0d7d44ae4ccfda9e2c7554f1/__docs__/wonder-blocks-icon/accessibility.mdx
Apart from that, the documentation is super clear and concise :)
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.
Updated to use mdx for the docs portion, thanks for the suggestion!
* screenreaders so that it can communicate the relationship between the label and | ||
* the form field. | ||
* | ||
* Guidelines: |
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.
suggestion/nit: After moving this to a MDX file, this could be formatted as a subheading (e.g. ### Guidelines
).
}; | ||
|
||
/** | ||
* <h2>Form Field Labels</h2> |
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.
suggestion: Same here: ## Form Field Labels
.
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.
Looks great! thanks for the MDX change
tags: [ | ||
"!dev", // Hide individual stories from sidebar so they are only shown in the docs page. | ||
], |
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.
Neat, TIL about this :)
Summary:
Issue: WB-1737
Test plan:
tag
prop for Typography components (?path=/docs/packages-typography--docs
)?path=/docs/packages-form-accessibility--docs
)?path=/docs/packages-typography--docs
)