-
Notifications
You must be signed in to change notification settings - Fork 47
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
Web Docs - Stepper Component #2727
base: dchyun/stepper-component
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
0a8d3e2
to
da2209c
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.
It's a good start. I think the separation between the two pages also helps make it more clear that these have two distinct use cases. I left quite a few suggestions and a few comments, but nothing too major.
website/docs/components/stepper/list/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/list/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/list/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/list/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/specifications/anatomy.md
Outdated
Show resolved
Hide resolved
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 haven't gotten through all of this yet, but I'll take a stab at the Nav
variant tomorrow.
website/docs/components/stepper/list/partials/code/how-to-use.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/list/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/list/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/list/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/list/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/list/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/code/how-to-use.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
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.
Ok I got through the rest of this now, dropped some more notes and some non-blocking grammar suggestions. LMK if there's anything that needs clarification!
website/docs/components/stepper/list/partials/accessibility/accessibility.md
Outdated
Show resolved
Hide resolved
|
||
Focus on the active step. | ||
|
||
 |
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.
[question] Are these screenshots from the Showcase? I'm questioning whether why the width is explicitly set to 1400px, which I don't think they will ever render at. Instead, should those screenshots be dropped into (or recreated in ) Figma and resized? For full-bleed images we generally size images at 836px (I know, also somewhat arbitrary), but that largely removes the need to set the width in markdown explicitly
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 this one is on me. I was following what was done for the tabs accessibility docs and there they set what seemed an arbitrary width of 402px on the images. I also picked 1400 arbitrarily. I removed explicitly setting the width for now.
I made these examples locally to show the interactions. I'm not sure what the standard way we make these kind of examples is, but if it's preferable to create them in Figma instead we can do that.
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, It's somewhat of an ambiguous "process", happy to pair with you on it if that would help, but @majedelass is also privy on the dimensions
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.
@majedelass I removed explicitly setting the width on the images. Do you think they need any other updates? Or do you want to recreate them in figma and change the example used?
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/overview.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
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.
@majedelass @dchyun I think we are getting close but could you both please review the current suggested edits and apply them if you agree? (Or else tweak them?)
Then I think we can wrap this up.
|
||
## Interactive vs non-interactive | ||
|
||
The Stepper Nav supports both interactive (default) and non-interactive variants. |
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.
[Issue] We may need to discuss this, but currently the nav is non-interactive by 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.
This is currently based off of the requirements doc in the variants section indicating that isInteractive
set to true
is the default.
Do we believe that our consumers will use the non-interactive variant more often than the alternative? That's usually how we define what is set by default.
website/docs/components/stepper/nav/partials/guidelines/guidelines.md
Outdated
Show resolved
Hide resolved
| Title | Required | | ||
| Description | Optional | | ||
| Body | Optional; accepts any custom component instance (local or from Helios)| | ||
| Progress bar | Required | |
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.
[Note] Not sure if this is worth documenting, but just fyi if there is only one step the progress bar will not be shown.
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.
If there's only one step, the Stepper shouldn't be used but maybe we could include an explanation in parenthesis in case that needs to be clarified.
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, the when to use section says "multi-step flow" meaning that it should never be 1 step only.
| Title | Required | | ||
| Description | Optional | | ||
| Body | Optional; accepts any custom component instance (local or from Helios)| | ||
| Progress bar | Required | |
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.
| Progress bar | Required | | |
| Progress bar | Required (not shown if only one step) | |
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 think we should not have this in our docs. We do not recommend folks to only have one step.
</Hds::Stepper::Nav> | ||
``` | ||
|
||
### Without panels |
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.
@shleewhite I added a section for using the nav without panels, which we've discussed earlier. Please let me know if the accessibility notes here look correct, or if any changes need to be made to the accessibility file as well.
Co-authored-by: Dylan Hyun <[email protected]> Co-authored-by: Kristin Bradley <[email protected]>
π Summary
If merged, this PR will add the Stepper component documentation to the website.
π οΈ Detailed description
The Stepper documentation is getting slightly restructured, where the previous Stepper Indicator will now live under a Stepper category, along side Stepper Navigation and Stepper List.
πΈ Screenshots
Before

After

π External links
Jira ticket: HDS-4405
π¬ Please consider using conventional comments when reviewing this PR.