-
Notifications
You must be signed in to change notification settings - Fork 95
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
feat(wizard): add wizard component #1404
Conversation
Deploy preview for pf-next ready! Built with commit 35839cb |
018d5fb
to
7c34c0d
Compare
This is looking good @mcoker . Just a few questions for you and @kybaker .
|
|
4c10bec
to
8577348
Compare
The non-current bg color is If you'd like me to use black-150, would this be a one-off use, or should that color be added as an additional global background-color var to use system-wide? |
Wow. That is pretty perceptible. @kybaker thoughts? |
{{/wizard-nav-item}} | ||
{{#> wizard-nav-item}} | ||
{{#> wizard-nav-link}} | ||
Subs-tep C |
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.
On a roll today... typo here.
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'm grate at teh spel-ling.
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 really good fantastic work!! Left some comments
|
||
// Close | ||
--pf-c-wizard__close--Top: calc(var(--pf-global--spacer--lg) - var(--pf-global--spacer--form-element)); | ||
--pf-c-wizard__close--Right: calc(var(--pf-global--spacer--xl) - var(--pf-global--spacer--md)); |
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.
Is there any reason why this isn't just var(--pf-global--spacer--md)
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.
Yep, that's a good idea. I originally wrote it this way to represent that the right value is the right padding of the container minus the right padding of the button. But unless we wrote it like --pf-c-wizard__close--Right: calc(var(--pf-c-wizard__header--PaddingRight) - var(--pf-c-button--PaddingRight));
, I don't think it makes sense to have this calc()
| -- | -- | -- | | ||
| `.pf-c-wizard` | `<div>` | Initiates the wizard component. | | ||
| `.pf-c-wizard__header` | `<header>` | Initiates the header . | | ||
| `.pf-c-wizard__close` | `.pf-c-button.pf-m-plain` | Initiates the close button. | |
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.
Since .pf-c-wizard__close
, .pf-c-wizard__title
and .pf-c-wizard__description
are nested inside of the header I feel as though their naming should be .pf-c-wizard__header-close
, .pf-c-wizard__header-title
and .pf-c-wizard__header-description
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.
We can, but per the BEM guidelines
If we were to add another Element—called, let’s say, .person__eye {}—to this .person {} component, we would not need to step through every layer of the DOM. That is to say, the correct notation would be .person__eye {}, and not .person__head__eye {}. Your classes do not reflect the full paper-trail of the DOM.
If there were more than 1 description, close, and title, it would make sense to use header-[element]
, but in this case there aren't. Even then, I think we could add .pf-c-wizard__footer-description
and leave .pf-c-wizard__description
and it wouldn't be confusing. Some of our classes get unnecessarily long :) But I'm fine with it either way. What do you think, still want to change it?
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.
Oh I never knew this I thought it was a hard rule. I'm definitely fine with leaving this!
| `.pf-c-wizard__description` | `<p>` | Initiates the description. | | ||
| `.pf-c-wizard__steps` | `<div>` | Initiates the steps container. | | ||
| `.pf-c-wizard__toggle` | `<button>` | Initiates the mobile steps menu toggle button. | | ||
| `.pf-c-wizard__toggle-label` | `<div>` | Initiates the label area in the toggle. | |
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.
The breadcrumb component uses <nav>
and a <list>
here. Since this is just for display purposes I get that the nav might not be needed but I think a list could work really well here rather than the spans.
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 call!
{{#> wizard-toggle-num}}2{{/wizard-toggle-num}} | ||
{{#> wizard-toggle-step}}Configuration{{/wizard-toggle-step}} | ||
{{> wizard-toggle-divider}} | ||
{{#> wizard-toggle-substep}}Substep A{{/wizard-toggle-substep}} |
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.
substep B has the pf-m-current modifier so might be good to update this text.
--pf-c-wizard__toggle--ZIndex: var(--pf-global--ZIndex--sm); | ||
|
||
// Toggle Label | ||
--pf-c-wizard__toggle-label--MarginRight: var(--pf-global--spacer--sm); |
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.
should this instead have a (max-width of 100% - width of icon), so that the text truncates rather than cutting off? Not sure what design calls for.
transform: rotate(180deg); | ||
} | ||
|
||
+ .pf-c-wizard__nav { |
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 might be worth checking with a react dev on this, but using the +
in this selector makes it dependent on HTML structure, and it would be ideal to have styles applied that are not dependent on HTML structure.
For example, something I'd like to test for components like this menu is if we can implement them in a way that is similar to how we handle popovers (wrt handling and trapping focus). Currently, the Popover displays in a separate section of the DOM from the toggle
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.
Yep, good call! Updated the toggle/menu expanded styles to respond to a class on their parent element instead that can be toggled in react.
{{#> wizard-footer}} | ||
{{#> form}} | ||
{{#> form-actions}} | ||
{{#> button button--modifier="pf-m-primary"}} |
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'm not sure if this is easy to do, or something we want to show here, but when I use this component in my product, I think I would want this button to be type="submit"
so that I can hit Enter in the context of the form to move on to the next page. Is that something we would want to document here, or is that something that we handle when building the React component?
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 agree. Do you think type="submit"
should be optional, or can we just ship it that way? Seems to me like the primary action in the wizard (including if it changes from "next" to "review" or "finish") should always have type="submit"
, so I would be fine just adding it. We do that with the Login component's form button currently. It's type="submit"
in core and react, but not documented in either. Do you think this (and the login form button?) should be documented as needing type="submit"
?
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 can't think of a reason why you wouldn't want type="submit"
(or if there's a reason to use input type="submit"
instead) so I think it would be fine to just ship it that way.
I don't think it hurts to document that, but at the same time, I don't know that we need to document it—it seems like basic, standard HTML stuff and I'm not sure where that would live in our current document structure.
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.
Cool! Added that in my last commit.
Thanks @christiemolloy @jgiardino, made those updates. |
{{/wizard-nav-link}} | ||
{{/wizard-nav-item}} | ||
{{#> wizard-nav-item}} | ||
{{#> wizard-nav-link wizard-nav-link--modifier="pf-m-current"}} |
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.
Similar to our Nav component, we should include aria-current="page
for the current selected page.
<button class="pf-c-wizard__toggle{{#if wizard-toggle--modifier}} {{wizard-toggle--modifier}}{{/if}}" | ||
aria-expanded="{{#if wizard--IsExpanded}}true{{else}}false{{/if}}" | ||
aria-controls="{{id}}-nav" | ||
aria-haspopup="true" |
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'm finding that this attribute in JAWS is really weird when you use it outside the context of a form. For example, if I'm navigating a form in JAWS, the components we have with aria-haspopup
work great. But if you encounter this component outside of a form, it's behavior is a little janky.
I'm hoping that what we have labeled as "Basic Dropdown" (without aria-haspopup, and implemented with similar keyboard interaction of a popover) will work for these use cases of Dropdowns and Selects that are not embedded in a form, but I haven't been able to test a POC of that to know for sure. For now, I'd suggest just keeping aria-expanded
and removing aria-haspopup
.
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.
Aaaaand I've been commenting on all the PRs that I think we can remove aria-controls
from cases like this. I think we only need it for things like Tabs or buttons in a toolbar, where the controlled element is not immediately after the toggle (or focus isn't automatically shifted like in a modal or popover). Currently this attribute only seems to benefit JAWS users, where JAWS provides a way to navigate to the controlled element.
thanks @jgiardino! made those updates. |
@christiemolloy updated |
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.
LGTM! Thanks!
{{#> wizard-main}} | ||
<p>wizard step content</p> | ||
{{/wizard-main}} | ||
{{#> wizard-footer wizard-footer--modifier="pf-m-align-right"}} |
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.
What is the reason behind the footer switching alignments across the two examples?
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.
To show the modifier class in use. Do you think it shouldn't?
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 would confirm with design that it the position of these buttons can vary
} | ||
} | ||
|
||
.pf-c-wizard__toggle-list { |
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.
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.
fixed
|
||
.pf-c-wizard__main { | ||
flex-grow: 1; | ||
padding-top: var(--pf-c-wizard__main--PaddingTop); |
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.
From what I can see in the inVision files it looks as though theres a larger padding size on desktop and then on mobile you need to increase the padding top. It would be helpful to get the actual specs from Kyle.
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'll setup a meeting with @kybaker and go over all of the spacing and the dropdown menu height (that's matched from the context selector)
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 the spacing. The header left/right padding, main padding, and nav padding should match the page component.
@@ -0,0 +1 @@ | |||
<i class="fas fa-chevron-down pf-c-wizard__toggle-icon" aria-hidden="true"></i> |
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.
The toggle icon looks different in the invision file
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.
fixed
|
||
// Nav | ||
--pf-c-wizard__nav--Top: 100%; | ||
--pf-c-wizard__nav--MaxHeight: #{pf-size-prem(200px)}; |
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.
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.
Talked to @kybaker and just allowing this to consume the available area when the nav is open.
} | ||
|
||
// Nav List (nested) | ||
--pf-c-wizard__nav--nested--MarginLeft: calc(var(--pf-c-wizard__nav-link--PaddingLeft) + var(--pf-global--spacer--md)); |
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.
Where is this variable - --pf-c-wizard__nav-link--PaddingLeft
. Might be wrong but I can't see it
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.
nice, the build was failing because of the missing var :) that's a leftover from a previous change. addressed that and another var.
--pf-c-wizard--GridTemplateRows: auto auto 1fr auto; | ||
--pf-c-wizard--md--GridTemplateRows: auto 1fr auto; | ||
--pf-c-wizard--md--GridTemplateColumns: 300px 1fr; | ||
--pf-c-wizard--Width: calc(100% - var(--pf-global--spacer--xl) * 2); |
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.
Does it need a width and height?
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 similar to a modal, so I think it would. That said, I'll verify with Kyle that it will always present as a modal, and assuming it fits in a .pf-l-bullseye
I'll make sure the height/width works the same as a modal
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.
@kybaker said to match about modal box sizing. updated to match.
--pf-c-wizard__description--Color: var(--pf-global--Color--400); | ||
|
||
// Steps | ||
--pf-c-wizard__steps--md--BoxShadow: var(--pf-global--BoxShadow--md-right); |
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.
does the md
refer to the box shadow size?
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 because it's a var in the md
media query: @media screen and (min-width: $pf-global--breakpoint--md)
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.
oh nice!
--pf-c-wizard--lg--GridTemplateColumns: 300px 1fr; | ||
--pf-c-wizard--Height: 100%; | ||
--pf-c-wizard--Width: 100vw; | ||
--pf-c-wizard--lg--Height: #{pf-size-prem(762px)}; // Shares height with about modal |
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.
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.
fwiw, your screenshot is of the normal modal, not the about modal.
@kybaker what do you think? This is on the edge case that a person has a super short viewport.
Here are some screenshots:
normal height
super short height
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 the way the wizard responds is ok. @christiemolloy We could open an issue to investigate the modal though.
{{/wizard-steps}} | ||
{{#> wizard-main}} | ||
{{#> wizard-body}} | ||
wizard content goes were |
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.
here*
{{/wizard-body}} | ||
{{/wizard-main}} | ||
{{#> wizard-footer}} | ||
{{#> form}} |
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.
Should we remove the form from the footer and add custom spacing
| Class | Applied To | Outcome | | ||
| -- | -- | -- | | ||
| `.pf-c-wizard` | `<div>` | Initiates the wizard component. **Required** | | ||
| `.pf-c-wizard__header` | `<header>` | Initiates the header . **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.
change to - header.
| `.pf-c-wizard__nav-list` | `<ol>` | Initiates a list of steps. **Required** | | ||
| `.pf-c-wizard__nav-item` | `<li>` | Initiates a step list item. **Required** | | ||
| `.pf-c-wizard__nav-link` | `<a>` | Initiates a step link. **Required** | | ||
| `.pf-c-wizard__main` | `<main>` | Initiates the main container. **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.
Could .pf-c-wizard__main
take on the padding of .pf-c-wizard__body
. I don't see styles for .pf-c-wizard__main
so I don't see the need for both of these classes, but let me know if I'm wrong :)
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 talked to @kybaker and we're treating this similar to a card. With the body element, you get the proper spacing that allows you just to put content in it and it works. Without that element, the main area is just a container where you can put anything you want, and we don't impose padding or anything.
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 that sounds good to me
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.
Nice work!!!
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 solid! Only a few comments :)
Also, I've noticed in the design the expanded steps don't take full height in mobile but they do in yours, was this changed?
--pf-c-wizard__toggle-list-item--MarginBottom: var(--pf-global--spacer--xs); | ||
|
||
// Toggle Number | ||
--pf-c-wizard__toggle-num--MarginRight: var(--pf-global--spacer--sm); |
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 isn't being used
| `aria-expanded="true"` | `.pf-c-wizard__toggle` | Indicates that the steps menu is visible. **Required** | | ||
| `aria-expanded="false"` | `.pf-c-wizard__toggle` | Indicates that the steps menu is hidden. **Required** | | ||
| `aria-label="close"` | `.pf-c-wizard__toggle-icon` | Gives the close button an accessible name. **Required** | | ||
| `aria-hidden="true"` | `.pf-c-wizard__toggle-icon`, `.pf-c-wizard__toggle-divider` | Hides the icon from assistive technologies. | |
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.
Should this be required?
{{#> wizard}} | ||
{{#> background-image}}{{/background-image}} | ||
{{#> wizard-header}} | ||
{{#> button button--modifier="pf-m-plain pf-c-wizard__close" button--attribute='aria-list="Close"'}} |
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.
aria-label="close"?
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.
doh!
--pf-c-wizard__toggle-list--MarginBottom: calc(var(--pf-c-wizard__toggle-list-item--MarginBottom) * -1); | ||
|
||
// Toggle List Item | ||
--pf-c-wizard__toggle-list-item--not-first-child--FontSize: var(--pf-global--FontSize--sm); |
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 16px in the design and the first child is actually 18px. You could target just the :first-child
though and use --pf-global--FontSize--lg
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.
oh good catch!
--pf-c-wizard--lg--GridTemplateColumns: 300px 1fr; | ||
--pf-c-wizard--Height: 100%; | ||
--pf-c-wizard--Width: 100vw; | ||
--pf-c-wizard--lg--Height: #{pf-size-prem(762px)}; // Shares height with about modal |
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 the way the wizard responds is ok. @christiemolloy We could open an issue to investigate the modal though.
|
||
// Reset styles for nested nav links | ||
.pf-c-wizard__nav-link { | ||
padding-left: 0; |
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.
do you need to reset padding here? I don't see it being set anywhere else.
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.
nope! that's leftover from a previous iteration.
.pf-c-background-image { | ||
&::before { | ||
position: absolute; | ||
top: 0; |
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.
Shouldn't you only need to redefine position
here?
{{#> wizard wizard--IsExpanded="true"}} | ||
{{#> background-image}}{{/background-image}} | ||
{{#> wizard-header}} | ||
{{#> button button--modifier="pf-m-plain pf-c-wizard__close" button--attribute='aria-list="Close"'}} |
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.
aria-label="close"?
thanks @matthewcarleton! updated. |
nice! Did you see my comment above too?
|
@matthewcarleton refactored the layout to support the mobile expanded nav state. mind looking at it again? |
@mcoker ya I think that works. Although did you notice that you can scroll the content behind outside of that area? So if I drag overtop the expanded menu it still scrolls the content. This is only an issue when the expanded area isn't taking full height. |
@matthewcarleton today I learned that webkit-overflow-scrolling: touch; creates a new stacking context, but it seems to ignore After spending way too much time trying workarounds I saw posted online and having no luck, I just removed |
fixes #755