-
Notifications
You must be signed in to change notification settings - Fork 230
docs(divider): update documentation #5629
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
Conversation
🦋 Changeset detectedLatest commit: a029b3e The changes in this PR will be included in the next version bump. 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 |
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
4d44c8d
to
2be8487
Compare
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 looks awesome! I've been adding a link to the design docs at the bottom of the overview section for convenience, but not a requirement by any means. ✨
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 solid! Can you also consider adding a table and list all available attributes (e.g., size, vertical, static-color) with a brief description and possible values?
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.
Let's check another one off of Nikki's table! I only had real small comments about lowercasing (is that a verb? I'm making it a verb 😆) a few words, and after Rajdeep's comments are addressed I think it's ready to merge.
Thanks!
import { Divider } from '@spectrum-web-components/divider'; | ||
``` | ||
|
||
## Horizontal | ||
### Options |
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 know anatomy is usually in the docs, but...I don't even know what we'd put there! 🤣 I'm fine with leaving it out.
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.
Yeah, there's no anatomy section in the design docs for this one 🤷♀️
packages/divider/README.md
Outdated
## Vertical | ||
#### Vertical | ||
|
||
Vertical dividers are used to separate content horizontally. | ||
|
||
When a vertical Divider is used inside of a flex container, use `align-self: stretch; height: auto;` on the Divider. |
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 is a pretty small thing, but should we use lowercase in "Divider" here? It doesn't need a capital D.
packages/divider/README.md
Outdated
- **ARIA Role**: Automatically sets `role="separator"` to ensure proper semantic meaning for assistive technologies | ||
- **Orientation Support**: When `vertical` is true, automatically sets `aria-orientation="vertical"` to indicate the divider's orientation |
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.
Super small nit-pick: the second word in these bolded bullet points don't need to be capitalized.
/** | ||
* The static color variant to use for the divider. | ||
*/ | ||
@property({ reflect: true, attribute: 'static-color' }) | ||
public staticColor?: 'white' | 'black'; | ||
|
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.
Can you also consider adding a table and list all available attributes (e.g., size, vertical, static-color) with a brief description and possible values?
@Rajdeepc Adding this would add staticColor
to the "Attributes and properties" table that's under the API tab, is that what you meant? It doesn't seem like these typically include sizes, unless there was an example somewhere that does this that I might have missed?
Because it seems like adding public staticColor
is the way to get it into the table, I've added it here, is that within scope for this kind of docs work? I also made some test updates/Storybook updates but could roll those back if that's not what we want.
dab7c0b
to
dd2217a
Compare
Description
Updates the divider docs page to meet documentation standards.
Motivation and context
Improving the accessibility documentation of the divider component, bringing formatting up to date with other freshly documented components.
Related issue(s)
Screenshots (if appropriate)
Author's checklist
I have added automated tests to cover my changes.I have included a well-written changeset if my change needs to be published.Reviewer's checklist
patch
,minor
, ormajor
featuresManual review test cases
Descriptive Test Statement
Descriptive Test Statement
Device review