-
Notifications
You must be signed in to change notification settings - Fork 355
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): adds the pf wizard and modal components #69
Conversation
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 didn't have a lot of time to review - but first glance seems awesome :)
a couple of concerns:
- no tests yet (beside snaphosts)
- pr is pretty large - maybe easier to get rid of the other stuff in smaller pr's - this will make reviewer life easier.
- a lot of custom logic in the components, I wonder in redux land if we would like to use it like that?
thanks!
src/Wizard/Wizard.js
Outdated
activeStepIndex: props.initialStepIndex || 0, | ||
activeSubStepIndex: props.initialSubStepIndex || 0 | ||
} | ||
this.onSidebarItemClick = this.onSidebarItemClick.bind(this) |
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.
maybe use the helper at https://github.com/patternfly/patternfly-react/blob/master/src/common/helpers.js#L2
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.
+1
src/Wizard/Wizard.js
Outdated
constructor(props) { | ||
super(props) | ||
this.state = { | ||
activeStepIndex: props.initialStepIndex || 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.
would this work with redux?
src/Wizard/Wizard.test.js
Outdated
* react-overlays / Jest opened issue | ||
* Open issue: https://github.com/react-bootstrap/react-overlays/issues/225 | ||
*/ | ||
// test('Wizard modal renders properly', () => { |
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.
csn use xit instead to comment out the code. (not sure whats better tbh)
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 looks like I can remove this test and convert to Enzyme now
less/patternfly-react.less
Outdated
margin-left: 5px; | ||
} | ||
} | ||
} |
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 naming of these should indicate that they are used for embedded wizards. These don't seem to match the embedded wizard designs http://www.patternfly.org/pattern-library/communication/wizard/#/design#embedded-wizard
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.
Sure - am hoping to working thru the CSS contribution into PF Core soon w/ @jgiardino . I will tag you on this!
src/Wizard/Wizard.js
Outdated
})) | ||
} | ||
} | ||
|
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 the buttons allow for the application to process the click and allow it to stop the navigation should there be an error or if there is a long running task associated with the navigation?
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 am working under the assumption that a "higher level" component in an application which is connected to the Redux store would manage this state and feed it down appropriately to these stateless components, so yes!
src/Wizard/WizardSidebar.js
Outdated
return ( | ||
<ListGroupItem className={classes} listItem {...rest}> | ||
<a | ||
href="#" |
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 it ok to use href="" here to avoid any navigation or browser history changes?
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.
+1, i believe it is.
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.
actually...I believe this is OK as long as the event is handled, i.e. preventDefault()
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.
In OpenShift, they are moving towards replacing anchors with buttons styled with btn-link to provide better accessibility. Is that something we want to consider here (and in general)?
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.
@jeff-phillips-18 i think both are acceptable from what I've seen before. Do you have an example some place? I can try to mimic the anchor with a button and see how it goes...I noted that the ListGroupItem would support buttons.
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.
Aside from saying that semantic html is accessible for free, I'm not aware of any standards that we have. Nor am I aware of what screen readers we should support. For now, I think the best thing we can do is to test using the devices we have access to.
Here is an article in favor of using <button>
since accessibility is enabled by default: https://www.deque.com/blog/accessible-aria-buttons/
The article also provides details on how to enable an <a>
element to be accessible using additional javascript if we are tied to using <a>
.
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 believe the ideas is to not require the call to preventDefault and to show the focus border when the item has focus.
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 will try the button and see if it impacts styling within the current PF styles for this 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.
Ok a quick browser test shows that Buttons do not have styles here.. i.e.: we have styles for:
.wizard-pf-sidebar .list-group-item>a
but not: .wizard-pf-sidebar .list-group-item> button
it seems.
So are we OK w/ using an <a>
, or should we halt until pushing some additional CSS?
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.
Yes, I would continue with using for now. PF core is currently working on a new wizard, so we can switch to that when it's available.
The article I mentioned also talks about what is needed when using <a>
instead of <button>
, such as using .preventDefault() (although that may not be needed if we using href=""
without the #
?).
(and I fixed the link to that article that I pasted in my previous comment)
@ohadlevy great questions that I think provide some important discussion points for this repo. Here is my thoughts at this point...
|
After reviewing the html/css of the modal wizard, I don't think the embedded wizard is going to be a simple modification to the css of the modal wizard. I created an issue against patternfly to track this work. |
src/ListGroup/ListGroup.js
Outdated
@@ -0,0 +1,2 @@ | |||
import { ListGroup } from 'react-bootstrap' | |||
export default ListGroup |
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.
IIUC It should be possible to simply just re-export this in src/index.js
export { ListGroup } from 'react-bootstrap'
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 we not prefer to break these out into their own individual stories and give PatternFly design examples (like in #74)? I think this is a good discussion point here on what we want to do with these kind of components that we are inheriting/exporting directly. My general thought is that it is good to detail them in stories if we can (and show different use cases that help the downstream consumers), but I'm not opposed to just exporting them.
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 PF designers are used to seeing all of the components demo'd like we have in the test pages... thus the question ;)
https://rawgit.com/patternfly/patternfly/master-dist/dist/tests/
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 believe this also follows the existing approach with MenuItem so if we change it, we should go back and update it too?
src/ListGroup/ListGroupItem.js
Outdated
@@ -0,0 +1,2 @@ | |||
import { ListGroupItem } from 'react-bootstrap' | |||
export default ListGroupItem |
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.
same as above
src/Modal/Modal.js
Outdated
@@ -0,0 +1,2 @@ | |||
import { Modal } from 'react-bootstrap' | |||
export default 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.
same as above
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 looking good so far, looking forward to using this.
src/Wizard/Wizard.stories.js
Outdated
<Row> | ||
<Col sm={12}> | ||
<Wizard | ||
className="wizard-pf" |
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 this something we should default to in the wizard itself?
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.
@waldenraines let me respond to this one after I update the PR later today... and let's see what gives.
Noticed that clicking on the step in the header takes you to the first substep for that step even if you are on a substep of the step. It should not navigate if you are already on one of the substeps of the step. |
@jeff-phillips-18 good catch 👍 . Will update this PR today... |
i've rebased this component based on #76 (Modal) and updated a few things:
In general - I think to keep things moving quickly for now, we can focus on Snapshot tests on our simple stateless functional components here in PF React (the JS APIs have become much more flexible downstream w/ the approach taken here thus far, so the APIs don't have as much interaction state to test in this repo). thoughts? |
const { activeStepIndex, activeSubStepIndex } = this.state | ||
|
||
return ( | ||
<Wizard className="wizard-pf"> |
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.
Currently, this class wizard-pf
sets the width of the wizard to 900px wide, which doesn't make sense for the embedded wizard. Is it possible to remove this class for the embedded wizard storybook example?
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 - after hearing your's and @waldenraines feedback, I am thinking we can have <Wizard embedded>
optional boolean prop which tells Wizard what the default class is and feeds down to child components interested. This should get us there...
src/Wizard/WizardTemplates.js
Outdated
const classes = cx('wizard-pf-header', className) | ||
return ( | ||
<div className={classes} {...rest}> | ||
<h4 className="wizard-pf-title">{title}</h4> |
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 are your thoughts on making minimal class changes to better align the UI with what's shown here: http://www.patternfly.org/pattern-library/communication/wizard/
If it's simple enough to have two variations of the header, one for embedded and one for modal, then here are some changes you could make to get the embedded version closer to the pf design example:
- remove the classes
wizard-pf-header
andwizard-pf-title
. - use an
<h2>
instead of<h4>
With these changes, there will still be a border which isn't included on the design example of the embedded wizard, but I'm not seeing where that's getting applied.
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 - I'll be happy to review this on Monday w/ you @jgiardino
I made a few comments about the embedded wizard and some changes that could get it closer to what the design page shows. I know we have an issue in patternfly to provide css for the embedded wizard, but until that's ready, there may be a few more minor adjustments we can make to this implementation to get it even closer than what I suggested above, if that's something you're interested in. Other than the css, one thing I noticed is that the keyboard navigation could be improved by placing focus in the first field on the form after navigating to a new step. We can always capture that as a future issue if it's not a simple task. |
f3529f7
to
f9ba919
Compare
After reviewing again to see if there are any other changes to better align the embedded wizard with the design documentation, I think the margins would be the only issue that we should try to address. I don't think we should attempt any other layout changes at this time, like layout of the footer actions to left-align them, and instead should wait for the embedded wizard pattern to become available in core patternfly. However, to fix the margin, I need to see an example of how the embedded wizard would display in the context of an app (e.g. with the masthead and navigation). So that I know if the margin needs to be added to the title or removed from the sidebar. This shouldn't block us from merging, though. I can add this update in a separate PR. I also created an issue in patternfly to address the lack of indication when a secondary step has focus: patternfly/patternfly#869 |
thanks @jgiardino @jeff-phillips-18 . I plan to comment out the Embedded story for now and iterate on that pattern as well as the a11y approach in future PRs if that is acceptable. I am finishing up the Wizard Review steps today and would like to consider merging this PR tomorrow after that last update is done. |
6a7d16f
to
3d05f53
Compare
update: I've added the relevant components for review steps (see step 3a in Storybook now). Commented out embedded example story for now (will follow up on this after PF Core issues resolve). @jeff-phillips-18 @jgiardino @sharvit @waldenraines what else 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.
Good job @priley86
Added a few comments
less/patternfly-react.less
Outdated
margin-left: 5px; | ||
} | ||
} | ||
} |
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 it would be easier to maintain if you separate it into a different file wizard.less
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 completely... I think after reviewing it again, it is OK to remove that temporary Less for now (as plan is to push it to Core). Will update it...
src/components/Modal/index.js
Outdated
@@ -0,0 +1,2 @@ | |||
export { default as CustomModalDialog } from './CustomModalDialog'; | |||
export { default as Modal } from './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 am not sure I get it.
- What is the difference between Modal and CustomModalDialog?
- What so custom in this modal so it called CustomModalDialog?
- Why do consumers need to consume both of them? Can't we have one configurable 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.
This one is not obvious...
The problem w/ the About Modal pattern I was having was being able to add an additional class to the modal-content
div (from Bootstrap). Currently, there is no simple way in the React Bootstrap API it seems other than providing a dialogComponentClass
and extending the Dialog. I looked at several issues on this and this seems like the most flexible option...
react-bootstrap/react-bootstrap#1940
I prefer to have a JS API rather than having to carry around additional CSS like that issue suggests.
I also tried a lot of other options, but there is not a clear way with current RB apis. My guess is extending the Dialog like that and providing an additional contentClassName
prop to extend the modal-content
div with our own classes will be useful. I am exposing CustomModalDialog b/c the About Modal pattern will be common (and maybe there are other scenarios you want to extend modal-content
too).
Long story short, I think this is likely a good enhancement upstream if the React Bootstrap team will accept one. This seems the best option for now...
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 understand the issue now, thanks!
What about implementing our own Modal component, that will receive some custom props and will render the Modal (with the CustomModal) appropriately?
Maybe even a prop call patternflyModalStyle
that will choose between of them (and can support more styles).
I just prefer to consume only one Modal, flexible as possible.
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.
+1 - should be easy enough. I don't think there is any issue w/ always using Custom Dialog but will test it... Custom Dialog will always be more flexible. We can move this to InnerComponents now.
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.
I've made the updates to extend RB Modal w/ CustomDialog by default. This will allow us to always customize the modal-content
div or anything else in the Modal Dialog we want.
</div> | ||
), | ||
})(() => <MockAboutModalManager />), | ||
); |
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 don't like the fact you are overriding the text, we are losing tons of benefits.
Some times something like this worked for me:
() => MockAboutModalManager()
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.
- same in wizards
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 👍 ! I will try it. This would be a huge relief...
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.
Any update?
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.
Unfortunately, it works only with stateless components
I guess there is no trivial solution, would be nice if storybook had an option to manually trigger knobs updates.
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.
@sharvit I think this might be something for us to consider in the future... not sure if it helps the React docgen/withInfo content... but it would help make the state more visible.
https://github.com/ndelangen/storybook-state
const Size = { | ||
LARGE: 'large', | ||
SMALL: 'small', | ||
XSMALL: 'xsmall', |
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.
XSMALL is never used
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.
true
XSMALL: 'xsmall', | ||
}; | ||
|
||
const propTypes = { |
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.
Why do wee need to store propTypes
as a variable? we never did it till now...
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.
+1
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
@jeff-phillips-18 @jgiardino last call here (am planning to merge this one soon if we can). I can't think of any more updates right now, but will try to tackle anything else before Monday if needed. I made notes about the open PF Core issues around a11y and embedded styles. Also planning to circle back later on the new mobile styles. It should be pretty easy to update this component for that later. patternfly/patternfly#869 |
The only issues I see in your implementation is the structure of the ListGroup and ListGroupItem components on the summary. In the runtime html, I'm seeing a structure like this:
Those two
You should instead have this structure: (plus the classes that you currently have)
|
@jeff-phillips-18 @jgiardino thanks for the reviews 👍 |
We have established a new convention in #95 which has address this.
Adds the PatternFly Wizard pattern
Would like to merge #74 (list-group) and #76 (modal) first, and then rebase this.
Storybook:
https://rawgit.com/priley86/patternfly-react/wizard-storybook/index.html
What:
This PR makes several changes:
wizard-pf
classes for embedded wizard pattern into pf-react temporarily until they are contributed to PF Core, cc: @jgiardinoListGroup
andListGroupItem
from React Bootstrap. Renames existingListGroupItem
implemented inListView
toListViewGroupItem
, cc: @jtomasekModal
. To support React 16 / portals, we must update react-overlays to0.7.3
(see issue here)Wizard
,WizardSidebar
, and related wizard templates.I am happy to split the bullets above into individual PRs that we can test and release as the community desires.