feat(wizard): add wizard step status#6447
Conversation
|
Preview: https://patternfly-pr-6447.surge.sh A11y report: https://patternfly-pr-6447-a11y.surge.sh |
| @@ -0,0 +1,6 @@ | |||
| <span class="{{pfv}}wizard__nav-link-icon{{#if wizard-nav-link-icon--modifier}} {{wizard-nav-link-icon--modifier}}{{/if}}" | |||
There was a problem hiding this comment.
I might rename this from icon to status-icon, just so we can leave room for a step icon that we can target uniquely from the status icon and not run into a namespace issue. WDYT?
|
|
||
| // Nav link status icon | ||
| --# 10BC0 {$wizard}__nav-link-icon--Color: var(--pf-t--global--icon--color--regular); | ||
| --#{$wizard}__nav-link--m-danger--nav-link-icon--Color: var(--pf-t--global--icon--color--status--danger--default); |
There was a problem hiding this comment.
Technically I think you'd use --m-danger__nav-link-icon (since it's a nested element) but since the var starts at __nav-link I think you'd also be alright just calling this __icon if you wanted to shorten the var.
| --#{$wizard}__nav-link--m-danger--nav-link-icon--Color: var(--pf-t--global--icon--color--status--danger--default); | |
| --#{$wizard}__nav-link--m-danger__icon--Color: var(--pf-t--global--icon--color--status--danger--default); |
There was a problem hiding this comment.
This is intended to replace how React currently renders error icons in the step error status example I assume?
| &::before { | ||
| // display: none; | ||
| opacity: 0; | ||
| } |
There was a problem hiding this comment.
Did we want to use the opacity or display property here?
| {{#> wizard-nav-link-status-icon}} | ||
| <i class="fas fa-exclamation-circle" aria-hidden="true"></i> | ||
| {{/wizard-nav-link-status-icon}} |
There was a problem hiding this comment.
Not a blocker here but something that might be good to do in the future, but instead of having to pass an icon to the status icon, maybe just rendering whatever icon in the status icon handlebars (instead of a partial-block) based on Is[status]
| {{#> wizard-toggle}} | ||
| {{#> wizard-toggle-list}} | ||
| {{#> wizard-toggle-list-item wizard-toggle-list-item--modifier='pf-m-danger'}} | ||
| {{#> wizard-toggle-status-icon}}<i class="fas fa-exclamation-circle" aria-hidden="true"></i>{{/wizard-toggle-status-icon}} |
There was a problem hiding this comment.
Similar to above comment that was resolved, not a blocker but could render the icon inside wizard-toggle-status-icon rather than needing to pass it in examples. Maybe even rendering the status icon inside of toggle-list-item when isDanger=true?
| {{#> wizard-toggle-list-item}} | ||
| {{#> wizard-toggle-num}}2{{/wizard-toggle-num}} | ||
| Configuration |
There was a problem hiding this comment.
Think this list item needs the status icon passed in as well. On wider viewport the 2nd step has an error icon, but on smaller viewport the toggle doesn't show the error icon. That or the 2nd step for desktop view should have the error icon removed (so that only the 1st step has an error).
Wider viewport:
Smaller viewport:
de5a1cc to
dc3604d
Compare
There was a problem hiding this comment.
We'll need a followup in React to adjust the error styling there to match, but otherwise looks good
There was a problem hiding this comment.
woot! Left some comments, lemme know what ya think!
| .#{$wizard}__nav-link-main { | ||
| display: flex; | ||
| flex-grow: 1; | ||
| justify-content: space-between; |
There was a problem hiding this comment.
I don't think you need this since there will always be nav-link-text, and it's set to flex-grow
patternfly/src/patternfly/components/Wizard/wizard.scss
Lines 508 to 510 in dc3604d
| --#{$wizard}__nav-link-status-icon--Color: var(--#{$wizard}__nav-link--m-danger__nav-link-status-icon--Color); | ||
|
|
||
| &::before { | ||
| display:none; |
There was a problem hiding this comment.
🤓
| display:none; | |
| display: none; |
| border-radius: var(--#{$wizard}__nav-link--BorderRadius); | ||
| } | ||
|
|
||
| .#{$wizard}__nav-link-status-icon { |
There was a problem hiding this comment.
One thing to note about the font awesome status icons is that the warning-triangle icon is wider than the icons we use for success/danger, so if we put a warning icon by a step name, that step text will be pushed over farther than the step text above/below it. It's not a huge issue since the icons in react are square, and most users will use those, but if a user supplied their own icon that wasn't a square, that may cause an issue.
I wonder if this element should share the shape size we define for the count circles, or we should reconsider using absolute positioning/translate here like it was before. Or possibly use subgrid for the number/status icon, though worth noting the column width would shift for all steps if a wider status icon was added to a step.
There was a problem hiding this comment.
@mcoker Can we defer this pending a decision about the new icon set?
|
|
||
| ### Error on step | ||
| ```hbs isFullscreen | ||
| {{#> wizard wizard--id="wizard-basic"}} |
There was a problem hiding this comment.
| {{#> wizard wizard--id="wizard-basic"}} | |
| {{#> wizard wizard--id="wizard-error-on-step"}} |
|
|
||
| ### Nav expanded with error (mobile) | ||
| ```hbs isFullscreen | ||
| {{#> wizard wizard--IsExpanded="true"}} |
There was a problem hiding this comment.
Looks like we're missing a wizard--id here and in the OG expanded example
Would you mind adding one here and in the other example? That's used for a few things, including the form that gets added to these wizards
| @@ -0,0 +1,6 @@ | |||
| <span class="{{pfv}}wizard__nav-link-main{{#if wizard-nav-link-main--modifier}} {{wizard-nav-link-main--modifier}}{{/if}}" | |||
There was a problem hiding this comment.
can you add this element to the component docs?
| | `.pf-m-expanded` | `.pf-v6-c-wizard__nav-item` | Modifies a nav item for the expanded state. | | ||
| | `.pf-m-current` | `.pf-v6-c-wizard__nav-link` | Modifies a step link for the current state. **Required** | | ||
| | `.pf-m-disabled` | `.pf-v6-c-wizard__nav-link` | Modifies a step link for the disabled state. | | ||
| | `.pf-m-danger` | `.pf-v6-c-wizard__nav-link` | Modifies a step link to indicate danger status. | |
There was a problem hiding this comment.
looks like you can also add it to the list-item in the mobile nav
| | `.pf-m-danger` | `.pf-v6-c-wizard__nav-link` | Modifies a step link to indicate danger status. | | |
| | `.pf-m-danger` | `.pf-v6-c-wizard__nav-link`, `pf-v6-c-wizard__toggle-list-item` | Modifies a step link to indicate danger status. | |
| --#{$wizard}__nav-link-status-icon--LineHeight: 1; | ||
| --#{$wizard}__toggle-status-icon--Color: var(--pf-t--global--icon--color--regular); | ||
| --#{$wizard}__toggle--m-danger__nav-link-status-icon--Color: var(--pf-t--global--icon--color--status--danger--default); | ||
| --#{$wizard}__toggle-status-icon--Top: calc(var(--pf-t--global--spacer--xs) / 2); |
There was a problem hiding this comment.
I could be wrong here, but it looks like this use of spacer--xs doesn't correspond with alignging the status-icon with something else that uses spacer--xs as a top spacer... this calc just returns 2px which looks to be about the adjustment that works visually to center the icon? If so, I might just use 2px here instead? It's really a nit, but if a user (or we as PF) themed/customized spacer--xs for a compact theme or whatever, I don't think we would want this value to change necessarily. I think the same comment applies to
Kind of a philosophical question around using a token because it gives the look you're going for, versus it being the right token given the token's meaning. Since our spacer tokens are so generic, I don't think it matters, but figured I'd leave a comment for light reading 😅
| padding-block-start: var(--#{$wizard}__nav-link--PaddingBlockStart); | ||
| padding-block-end: var(--#{$wizard}__nav-link--PaddingBlockEnd); | ||
| padding-inline-start: var(--#{$wizard}__nav-link--PaddingInlineStart); | ||
| padding-inline-end: var(--#{$wizard}__nav-link--PaddingInlineEnd); | ||
| background-color: var(--#{$wizard}__nav-link--BackgroundColor); | ||
| border: none; | ||
| border-radius: var(--#{$wizard}__nav-link--BorderRadius); |
There was a problem hiding this comment.
should we rename these?
| padding-block-start: var(--#{$wizard}__nav-link--PaddingBlockStart); | |
| padding-block-end: var(--#{$wizard}__nav-link--PaddingBlockEnd); | |
| padding-inline-start: var(--#{$wizard}__nav-link--PaddingInlineStart); | |
| padding-inline-end: var(--#{$wizard}__nav-link--PaddingInlineEnd); | |
| background-color: var(--#{$wizard}__nav-link--BackgroundColor); | |
| border: none; | |
| border-radius: var(--#{$wizard}__nav-link--BorderRadius); | |
| padding-block-start: var(--#{$wizard}__nav-link-main--PaddingBlockStart); | |
| padding-block-end: var(--#{$wizard}__nav-link-main--PaddingBlockEnd); | |
| padding-inline-start: var(--#{$wizard}__nav-link-main--PaddingInlineStart); | |
| padding-inline-end: var(--#{$wizard}__nav-link-main--PaddingInlineEnd); | |
| background-color: var(--#{$wizard}__nav-link-main--BackgroundColor); | |
| border: none; | |
| border-radius: var(--#{$wizard}__nav-link-main--BorderRadius); |
14e0648 to
b21ed86
Compare
b21ed86 to
139060e
Compare
|
🎉 This PR is included in version 6.0.0-alpha.123 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #5142
This adds an icon element and styles it to appear where the step number currently appears (hiding the step number if so).
A new modifier is added for danger status.
A new full page example is added to wizard.
The size of the step number was adjusted to match the xl icon size per @lboehling