feat(WizardPattern): Add WizardPattern and StatefulWizardPattern components #268

mturley · 2018-03-12T21:11:28Z

What:

Adding a new component for a Modal Wizard, building on the existing Modal and Wizard components. This is needed for ManageIQ's V2V POC project, and for other consumers from patternfly-react slack.

NOTE: This PR now includes breaking changes to Wizard. Now that we no longer support an embedded Wizard, we are simplifying things so Wizard always renders a Modal.

Link to Storybook (Updated 4/17 @ 6ead9a0):

https://rawgit.com/mturley/patternfly-react/wizard-pattern-sb/index.html?knob-Loading=false&knob-Active%20Step%20Index=0&selectedKind=Wizard%2FPatterns&selectedStory=Stateless%20WizardPattern%20Example&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybooks%2Fstorybook-addon-knobs

coveralls · 2018-03-12T21:14:36Z

Pull Request Test Coverage Report for Build 1207

85 of 85 (100.0%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.9%) to 76.162%

Totals
Change from base Build 1203:	0.9%
Covered Lines:	1605
Relevant Lines:	1922

💛 - Coveralls

priley86 · 2018-03-12T22:33:26Z

It looks like there is some recent news on the "controlled" pattern... https://twitter.com/brian_d_vaughn/status/972237949113835525

I recommend the following changes to introduce getDerivedStateFromProps:
priley86@b8625e3

You can test this storybook I deployed to see the same exact behavior...
https://rawgit.com/priley86/patternfly-react/modal-wizard-storybook/index.html

I am happy w/ this pattern if those tweaks are made (and I think it is much safer long term). The controlled pattern is very powerful for a lot of the state that is getting introduced here lately... FWIW though - I still think activeStepIndex can probably be tracked in Redux as well (and passed down as a prop via the ModalWizardContainer). Hope to demonstrate this in v2v after...

@AllenBW thanks for the tests examples... seems they work out fine!

priley86 · 2018-03-12T22:35:35Z

src/components/ModalWizard/ModalWizardBody.js

+    } = this.props;
+    // Don't allow step clicks to skip into the future, but skipping into the past is ok.
+    const nextStepClicked = stepIndex === activeStepIndex + 1;
+    const futureStepClicked = stepIndex > activeStepIndex + 1;


is this piece too opinionated? I think we have the case we want to prevent the user from going back on Step 5 in v2v...

There may be steps that are defaulted and can be skipped. I believe the application should decide if it is OK to navigate to a step.

Yeah. I didn't really finish figuring this part out because this behavior is totally turned off in v2v. I think I am going to add back in the disableGoto piece that we had added and then abandoned in v2v, so that you can turn these buttons off step-by-step, and like you say, the application can decide.

From Slack, my plan to address this flexibly is:

i'll have props for isValid, allowEnter and allowExit. allowEnter will default to the isValid of the previous step, but you can override it if you need to. we can use allowExit: false on our final step.

is this piece too opinionated? I think we have the case we want to prevent the user from going back on Step 5 in v2v...

In VM provision wizard, it should be possible to skip steps (jump to a later step).

There may be steps that are defaulted and can be skipped. I believe the application should decide if it is OK to navigate to a step.

Exactly my thoughts.

@mturley +1 on the proposed props for controlling the step navigation.

@vojtechszocs @jeff-phillips-18 @priley86 @AllenBW curious about your thoughts on the boolean props being available on the inner, stateless WizardPattern i.e:

<WizardPattern activeStepIndex={0} steps={[{ allowEnter: true, ... }, ...]} />

while on the stateful wrapper, we can pass something that will take the current step index as an argument (since when you're using the stateful wrapper, you probably are not tracking the current step) and still allow you to have logic based on the step index:

<StatefulWizardPattern steps={[ { shouldAllowEnter: (activeStepIndex) => { ... }, ... }, ... ]} />

Of course, all the props passed to StatefulWizardPattern are passed down into the stateless WizardPattern, so if you want to use allowEnter explicitly from there, you can do that. So even if we did a stateful wizard for v2v, we could still just pass allowExit: false instead of having to write shouldAllowExit: () => false. Should make it easier for people to switch/upgrade/downgrade.

I guess there is no reason for shouldAllowExit to take the current step index (edit: although maybe it should take the target step index). and shouldBeValid doesn't make sense that it could change for step A when you move from step B to C. so really i guess it's just shouldAllowEnter that needs the activeStepIndex information, and the others shouldn't be callbacks (edit: except maybe shouldAllowExit).

In either case, if you pass the simple booleans, that trumps any callback values. right?

Maybe the entire thing should instead be one large shouldStepChange(activeStepIndex, targetStepIndex) callback at the top? that'll distinguish it from onStepChanged better too.

Note: please don't reply here, I moved all of the above down to the main discussion thread for easy access.

priley86 · 2018-03-12T22:49:01Z

does anyone have thoughts on calling this a ControlledWizardPattern and putting it into Wizard directory? I'm not terribly opinionated on that... but it does seem we're going to blow up the current navigation quickly. We could also just call it ControlledWizard and place it below Wizard in Storybook (kind of how we did with Paginator/PaginationRow).

Wizard
--> Loading wizard
--> Modal wizard
--> Controlled Modal Wizard

Kind of still waiting on feedback in #256 from the community ... (and less opinionated on this right now)

jeff-phillips-18 · 2018-03-13T11:56:40Z

src/components/ModalWizard/ModalWizard.js

+              disabled={nextStepDisabled}
+            >
+              {onFinalStep ? closeButtonText : nextButtonText}
+              <Icon type="fa" name="angle-right" />


The > should not be shown on the close button

Thanks, fixed.

jeff-phillips-18 · 2018-03-13T12:01:16Z

src/components/ModalWizard/ModalWizard.js

+    } = this.props;
+    const currentStep = steps[activeStepIndex];
+    setControlledState({ activeStepIndex: newStepIndex });
+    if (onStepChanged) onStepChanged(newStepIndex);


onStepChanged should validate with the application prior to actually navigating. There are implementations where the next button (or navigating forward) causes some action which may need to be validated before navigation. The onStepChanged method should be able to return a boolean or a promise to validate the navigation.

Agreed, @jeff-phillips-18. we had implemented that validation with other props, like a disableGoto or a shouldPreventNavigation callback or boolean in each step of the steps array. or a callback at the component level that takes a step index argument and returns that boolean.

onStepChanged returning a promise is interesting-- but right now it's intended to be a callback for after the step changes, and won't be called at all if the step is prevented from changing. I'm cool with either pattern.

To be clear, the disableGoto was added to v2v after this refactor forked off-- i only didn't add it here because we ended up circumventing it anyway with stepButtonsDisabled. All the validation for navigating forward is currently on shouldDisableNextStep, a callback that you can pass in to ModalWizard that takes a step index. I was considering also making available a nextStepDisabled prop that could override that. I think that might be sufficient instead of using promises on the optional callbacks.

In case the user wants to add a completely custom button to the wizard's footer and/or further tweak existing button logic and/or add other custom stuff into the footer, it should also be possible to replace the footer with a custom implementation (in such case, the responsibility of handling footer content is shifted from ModalWizard to its user).

Good point @vojtechszocs. I'll have to add that into my next iteration.

jeff-phillips-18 · 2018-03-13T12:22:27Z

src/components/ModalWizard/ModalWizardBody.js

+    } = this.props;
+    // Don't allow step clicks to skip into the future, but skipping into the past is ok.
+    const nextStepClicked = stepIndex === activeStepIndex + 1;
+    const futureStepClicked = stepIndex > activeStepIndex + 1;


There may be steps that are defaulted and can be skipped. I believe the application should decide if it is OK to navigate to a step.

cdcabrera · 2018-03-13T14:53:40Z

Overall like the piece, I think it's good work @mturley that being said... @priley86 @mturley like the idea of breaking this out into a separate example/storybook piece. Partial to giving preference towards stateless, and leaving pieces such as this as an example.

Reasons...

It feels rare when a framework gives us the opportunity to create components without opinion, something that can be wrapped by an opinionated consumer downstream. The more we incorporate opinion into code the more we should be prepared to handle on the maintenance and issue sides. Leaving opinion to examples means we can still embrace Patternfly as an opinionated framework, but also embrace a more robust ability to upgrade framework versions.
Session storage. Embracing this as an example means we can limit opening ourselves up to helping resolve potential security issues. Incorporating session storage into any application should really come with security precautions.

Example... logging a user out of an application, and a second user without the same permissions logs into the same interface without closing the tab.

mturley · 2018-03-13T18:01:45Z

@vojtechszocs, I'm doing a little refactoring around disabling step navigation in certain conditions. I'll push changes in a little while, and also some better unit tests.

mturley · 2018-03-13T18:15:51Z

Hey @cdcabrera. I totally agree with the preference for stateless, and the concerns over session storage.

I do feel however that if we are going to have an opinionated example, we might as well provide it as a component. The stateful pieces are totally optional (we can pass a different activeStepIndex prop down, and it will override the values given to setControlledState). That is the idea behind the export controlled() HoC, to make a "reference implementation" of stateful behavior, that "just works" out of the box so users don't have to hook everything up to redux before they see something working, and then allow overrides of those state values with props, to put the component in a "stateless mode" when the consumer is ready to go there.

Maybe adding some console warnings in controlled.js when you are in a partially stateless mode (not passing all of the controlledState types as props) would help people along the way towards stateless.

Also, even if we have opinionated examples, we are going to see more fragmentation/variation if we are forcing everyone to copy and paste them every time (and then what if we want to update them, suddenly people's code is out of date). People are free to use the source of ModalWizard in their own alternative component, but I think the more we can reduce that and get people sharing code, the better. When prototyping, they can ignore state and use the built in state, and when lifting the state out properly, they have a consistent example right there in the code.

I definitely can go either way on this. My thinking was, enable what you are describing as an ideal, and support simpler use cases too for prototyping / avoiding the cathedral problem. I want to avoid making someone write a ton of redux code they might have to throw away before they can get feedback on the interaction in their components.

mturley · 2018-03-13T18:22:30Z

@cdcabrera note that we are planning to refactor controlled.js so it uses this pattern under the hood: https://twitter.com/brian_d_vaughn/status/972237949113835525 (or just use that pattern here instead of controlled.js. separate discussion ongoing on that)

mturley · 2018-03-13T18:29:58Z

@priley86 , one more thing on your comment i missed earlier:

does anyone have thoughts on calling this a ControlledWizardPattern and putting it into Wizard directory? I'm not terribly opinionated on that... but it does seem we're going to blow up the current navigation quickly. We could also just call it ControlledWizard and place it below Wizard in Storybook (kind of how we did with Paginator/PaginationRow).

I would be okay with that. That might better satisfy @cdcabrera's point that this is just an opinionated expression of other components, while still letting people take advantage of it as a higher-level component with a lower barrier to entry/setup.

mturley · 2018-03-13T18:37:52Z

Actually @priley86 I think if it's going to be ControlledWizardPattern we should also have a statelessWizardPattern that does not have the state behavior built in. And then ControlledWizardPattern should be a wrapper for that. Maybe that makes everyone happiest... @cdcabrera?

I don't think we should call it ControlledWizard since it's not just a controlled version of Wizard-- it does pull Modal in as well (as goes the "wizard pattern").

priley86 · 2018-03-14T13:17:48Z

@mturley yes - i like the distinction a lot as far as naming goes and great job mentioning this (that is the main idea I think and was trying to stay unopinionated on this). As long as we can distinguish "patterns" from simple stateless components I'm very happy... and taking it a step further with WizardPattern to ControlledWizardPattern makes even more sense to me (WizardPattern is still stateless, but it's a collection/abstraction of those)... but leave that to you to decide. Really appreciate the hard work on this!!

Regarding navigation - I think it makes sense to keep it underneath Wizard for now (if possible). We can move to tertiary navigation/hierarchy soon enough. There is even more great features coming with Storybook 3.4 soon and hope to help w/ that upgrade.
https://medium.com/storybookjs/storybook-3-4-early-access-964dee3d978e

vojtechszocs · 2018-03-14T15:30:25Z

👍 this is something I've been converging to in my VM provision wizard component as well.

Currently, Wizard and its parts (like Wizard.Body, Wizard.Row, etc.) are 1-to-1 reflections of the corresponding visual element. This means that users need to compose their logical wizard component out of many smaller physical components:

<Wizard>
  <Modal.Header />
  <Modal.Body>
    <Wizard.Steps />
    <Wizard.Row />
  </Modal.Body>
  <Modal.Footer />
</Wizard>

I see the ModalWizard as a higher level (logical) component, allowing users to focus on their specific use case, e.g. focus on the wizard steps and their content.

<ModalWizard
  steps={wizardSteps}
  .. />

@jgiardino mentioned that the "embedded" wizard UX pattern isn't planned to be supported for now, which means a follow-up cleanup could take place:

Wizard.Header, Wizard.Body and Wizard.Footer are not needed for "in-modal" usage
in Wizard container, the embedded prop and React.Children.map code is needed only for handling Wizard.Header and therefore those things aren't needed for "in-modal" usage

Users should be encouraged to use ModalWizard logical component instead of using Wizard bits mixed with Modal bits. This will ensure UX consistency and commonality across all wizard component applications.

I have some additional requirements, I'll implement these locally for now, later on we can decide if they're worth contributing to ModalWizard itself:

ability to override footer, in my case I'd like to have a "Customize Workflow" link on the left-hand side of footer that triggers an overlay that allows customizing which parts of the wizard are visible
probably more things as I proceed with my implementation

TL;DR this PR is a great start and will help adopting the wizard UX pattern. It doesn't have to be perfect right from the start, we can improve it over time.

@jelkosz please add any additional information if I missed anything.

vojtechszocs · 2018-03-14T15:54:00Z

I recommend the following changes to introduce getDerivedStateFromProps:
priley86/patternfly-react@b8625e3

It's always a good idea to stay in sync with changes in React APIs. If we can use getDerivedStateFromProps to handle component-specific UI state while being able to "lift it up" through props, that's great.

That said, I agree with @mturley that we could add this into controlled HOC creator (or add a new one if necessary) for now. In general, I'd avoid using class-based components and always prefer functional ones, following the main premise of Recompose:

aim for simple, functional, easy-to-test and easy-to-reason-about components
if we need more complexity (UI state handling, lifecycle methods, etc.) - use HOC creators that augment the passed in component in some way
each HOC creator is basically a separate concern, represented in a modular and easy-to-test way (i.e. each HOC creator should have a unit test that helps demonstrate its purpose and value)

@priley86 one question regarding above code, why use Enzyme to mount + render + toJson when one can simply use React to-string renderer? I always thought Enzyme is great when one wants to interact with and reason about the DOM structure, but for "renders correctly" use cases, isn't react-test-renderer better? (it's also faster as it doesn't need or touch the DOM env.)

priley86 · 2018-03-14T16:04:01Z

That said, I agree with @mturley that we could add this into controlled HOC creator (or add a new one if necessary) for now. In general, I'd avoid using class-based components and always prefer functional ones, following the main premise of Recompose...

@vojtechszocs i guess we have a minor disagreement on this piece (and apologies for confusion)... i prefer to not introduce more abstractions if a lifecycle method is already available (wasn't sure on this last week when it was discussed, but it seems that is the preferred approach moving forward). I see a lot of Redux/Recompose functionality moving into React Core (and I personally don't think we need to abstract those pieces anymore). Redux and Recompose will both be changing/simplifying to support future React Core APIs, so I prefer to be safe and use those Core APIs (also find them a lot simpler for beginners than HOCs). Same thing is about to happen with the Context API (React 16.3 will formalize that API and it will be having even more enhancements in the future ). So this would mean we would not need Recompose for sharing context either...
https://medium.com/@baphemot/whats-new-in-react-16-3-d2c9b7b6193b
https://www.youtube.com/watch?v=v6iR3Zk4oDY

FWIW I am sure that Redux/Recompose will be adjusting (and can post some discussions about that I saw recently), but I don't think it is good to target those APIs much longer for this sort of thing...

re: testing .. I am not super opinionated on that piece right now. @AllenBW maybe you have thoughts?

vojtechszocs · 2018-03-14T16:35:31Z

does anyone have thoughts on calling this a ControlledWizardPattern and putting it into Wizard directory? I'm not terribly opinionated on that... but it does seem we're going to blow up the current navigation quickly. We could also just call it ControlledWizard and place it below Wizard in Storybook (kind of how we did with Paginator/PaginationRow).

Once this PR is merged, ModalWizard should provide the same result as the "in-modal" wizard mock at src/components/Wizard/__mocks__/mockEmbeddedWizardManager.js (minus the actual wizard content, which might differ).

It makes sense to put ModalWizard right next to Wizard component, maybe also rename Wizard.stories.js to ModalWizard.stories.js. The fact whether ModalWizard is "controlled" or not could be determined by whether the user applies the relevant HOC creator or not, i.e. there could be a basic ModalWizard that gets all the stuff via props and ControlledModalWizard that returns HOC on top of it.

That is the idea behind the export controlled() HoC, to make a "reference implementation" of stateful behavior, that "just works" out of the box so users don't have to hook everything up to redux before they see something working, and then allow overrides of those state values with props, to put the component in a "stateless mode" when the consumer is ready to go there.

There are two types of state, one that's related to inputs (data/callbacks needed by the component) and one that people call "UI state", for example:

wizard "steps" is input-related state (always passed in via props)
wizard "active step index" is the UI state

You can either have a ModalWizard treating "active step index" as input-related and have ControlledModalWizard that treats it as UI state, overridable through props. Or, you can combine these two into one. From end user perspective, as long as you're in control, i.e. able to do the props override of UI state (=> allow for stateless component) I think it's not a big deal.

priley86 · 2018-03-14T17:40:57Z

@vojtechszocs

if we need more complexity (UI state handling, lifecycle methods, etc.) - use HOC creators that augment the passed in component in some way

I guess my only rebuttal to this is we just have a disagreement on that piece. One could still easily introduce an HOC downstream (if getDerivedStateFromProps were introduced), but I think putting those opinions in the PF React Core library is going to lead to more upgrade challenges and confuse beginners that are testing and maintaining. Agree that an additional functional wrapper can be written, but I don't see the point if React's Core APIs are being enhanced to prevent the need. You are trying to further abstract a Core lifecycle API for the sake of a few lines of code? To extend that further, should we abstract all React lifecycles...??? Also don't see the need to test and maintain those wrappers in PF React. Hope it helps explain my position a bit further...but will wait for others to weigh this.

If it's helpful for you...Material UI team just did the same upgrade...
https://github.com/mui-org/material-ui/pull/10523/files

...

Last bit i'll add here...

I certainly appreciate the healthy debate and thoughts being shared (it's a very good topic for this repo and pretty central to future implementations, so I am trying to look at this from both sides). I see the HOC pattern being used in this specific PR to apply derived lifecycle state, but from the comments above, it seems what's potentially being suggested is something different and additional (using an HOC to apply UI state lifecycle to a separate or multiple similar stateless components to augment those components or patterns). This same sort of abstraction could also be achieved with an es6 base class, however React's ecosystem tends to prefer the functional/HOC approach. An HOC need not be tied to Recompose (there are plenty examples of HOCs not tied to that library...Redux's core implementation is a great example of this). However, I do not see the use case of augmenting multiple components/patterns being presented currently in this PR.

FWIW - the approach w/ Recompose still adds the dependency and the additional maintenance, and should be used with caution if it is intended to replicate Core APIs arbitrarily in a way that would prevent future upgrades...additionally, I don't see a reason to abstract such things unless the abstraction is intended for multiple components, and the current BaseModalWizard in this PR uses a base class to handle that abstraction instead of an HOC. It's also intended to become generic enough that the other abstraction should not be necessary...

es6 base class vs recompose vs generic wrapper is the slope we face on this, for wrappers/ patterns / managers if we decide to introduce them here ... ⛷

AllenBW · 2018-03-14T19:53:31Z

First off @mturley THIS IS WONDERFUL and the discussion it's generating 😍 🙇‍♀️

OOh OOh a mention, replying to....

@priley86

re: testing .. I am not super opinionated on that piece right now. @AllenBW maybe you have thoughts?

which was a response to:

@vojtechszocs

@priley86 one question regarding above code, why use Enzyme to mount + render + toJson when one can simply use React to-string renderer? I always thought Enzyme is great when one wants to interact with and reason about the DOM structure, but for "renders correctly" use cases, isn't react-test-renderer better? (it's also faster as it doesn't need or touch the DOM env.)

@vojtechszocs the pr here does a bit to explain my preference of enzyme to react-test-render

tl;dr - less syntax, more flexibility, more meaningful snapshots

Chiming in on on a few architecture points...

I prefer to be safe and use those Core APIs

yes YES YES please for the sake of future us, (the following is an opinion) 3rd party libraries introduce maintenance complexity over time, using only what we need can make keeping future deps up to date much easier... (The pr mentioned above seems to contradict this position... but the backing of a large company assuages concerns of future volatility)

Patternfly designates wizards as either being modal or embedded, presently, we don't make this distinction well, (or have an embedded example for that matter) having an embedded/modal folder in the wizard component offering kinda makes sense

More comments to come... just getting this outta the way first 🌮 🥑 💃

mturley · 2018-03-15T03:55:46Z

@AllenBW , @priley86 , @vojtechszocs thank you guys all so much for your input. I'm really glad we're getting this much visibility on the right kinds of questions, in the right places, so quickly.

I have much to churn and process here, and feedback has pushed back the pace I was originally on here. Good problem to have! New commits with all of the above taken into account will come tomorrow. Let's get this thing done right.

mturley · 2018-03-15T04:08:14Z

To clarify, where I have landed after discussions around this controlled pattern, getDerivedStateFromProps, and the HoC pattern, is this: I'll have a stateless WizardPattern, a StatefulWizardPattern wrapping that which implements its state with getDerived..., and probably just renders as a WizardPattern instead of being a HoC for use with WizardPattern. The props interface for WizardPattern and StatefulWizardPattern will be identical except that in Stateful..., the ones corresponding to internal state will be optional.

Users can start with a StatefulWizardPattern and then when they are ready to lift state out, just pass in some additional props. When all the state is taken care of with props, the user can switch to WizardPattern with the same props and nothing will change (just the then-disabled state wrapper would be gone).

controlled can therefore be left out and killed off eventually, in favor of getDerivedStateFromProps being used in isolation in that stateful wrapper. No need for a HoC here anymore, to @priley86's point.

priley86 · 2018-04-05T20:06:37Z

I'm wondering if above qualifies for a Pattern, but in any case, it greatly simplifies handling of individual form groups (each group is basically a label with the corresponding input). I think other developers who have to deal with form stuff would also appreciate such components.

Fine w/ that pattern if others are, but pretty please, a separate PR :)

@priley86 @mturley please consider merging this PR, even if it's not perfect. We really need this in context of oVirt/KubeVirt and I'd like to avoid complex refactors due to subsequent PR changes, if possible.

I'm happy to see it merge if the following three things are done:

incorporate the new tests (cherry-pick the commit)
remove the polyfill - we are now targeting React 16.3 and getDerivedStateFromProps is already there. That upgrade happened during the course of this PR. I don't see a reason to use that here now unless others have an issue moving to 16.3. We can update our peerDependencies to React 16.3 as well.
add BREAKING CHANGE to the commit so semantic-release does a major bump.

How much effort would it take to use WizardPattern in V2V project? (Need to modify the current WizardPattern? Or just need to adjust V2V consumer code itself?)

This is currently on hold. We have other things that are more pressing than abstracting these API hooks. Our existing Redux Form flow is OK for now but we'd like to revisit after Summit. It should not block this PR though - I agree w/ you on that.

mturley · 2018-04-10T17:22:42Z

Okay guys. Sorry for the delay on this. I am now moving to knock out the things @priley86 listed in his comment above. We will get this merged ASAP. Sorry for the continued delay @vojtechszocs... this coincided both with a team meetup week and my PTO week. Lots of distractions.

priley86 · 2018-04-12T18:21:38Z

@mturley - any updates here?

mturley · 2018-04-15T19:07:41Z

@priley86, @vojtechszocs, My sincere apologies for how long it took me to get back around to this.

I've squashed everything and implemented the things in @priley86's checklist. There were some strange merge conflicts around the Wizard tests, so it would be nice to get a once-over on those.

I'm running into weird issues running npm install locally, which I am also getting on a fresh clone of master, so it must be something wrong on my machine. Waiting on Travis to see if maybe it passes anyway. But when I try to install I get this:

{
  "status": 1,
  "file": "/Users/mturley/tmp/patternfly-react/sass/patternfly-react.scss",
  "line": 9,
  "column": 1,
  "message": "File to import not found or unreadable: bootstrap/variables.",
  "formatted": "Error: File to import not found or unreadable: bootstrap/variables.\n        on line 9 of sass/patternfly-react.scss\n>> @import 'bootstrap/variables';\n\n   ^\n"
}

mturley · 2018-04-16T03:34:59Z

Looks like Travis is happy, so I guess I've got something wrong with my local environment. Will troubleshoot tomorrow.

I think we've addressed everything.. if not we can follow up with another PR

priley86 · 2018-04-16T15:20:11Z

i'm good to go here... thanks a lot for your diligence and patience w/ this one @mturley ! I think we can continue iterating on the patterns/storybook hierarchies in future PRs, but this gets the Wizard much further!!! Let's be sure to announce that we officially moved to React 16.3 at the next community meeting and this PR is the first to implement getDerivedStateFromProps... woohoo! 😸

mturley · 2018-04-17T15:45:07Z

Rebased, fixed merge conflicts, and updated storybook. @priley86 can you re-approve?

…izardPattern components BREAKING CHANGE: Wizard structure has changed

mturley · 2018-04-17T22:17:38Z

Rebased again and ran lint:fix.

priley86 · 2018-04-18T11:49:30Z

for fear that Github will no longer load these comments.... i am hitting the approve button now 😸

mturley · 2018-04-18T12:36:12Z

Let's merge! @jeff-phillips-18 maybe you can?

mturley self-assigned this Mar 12, 2018

mturley requested a review from priley86 March 12, 2018 21:11

jgiardino added the review label Mar 12, 2018

mturley force-pushed the modal-wizard branch 2 times, most recently from 140b467 to 653fcd0 Compare March 12, 2018 21:12

AllenBW self-requested a review March 12, 2018 21:40

priley86 reviewed Mar 12, 2018

View reviewed changes

jeff-phillips-18 reviewed Mar 13, 2018

View reviewed changes

mturley assigned mturley and unassigned mturley Mar 13, 2018

mturley mentioned this pull request Mar 13, 2018

Wizard abstractions ManageIQ/manageiq-v2v#136

Closed

priley86 added the structural pattern label Mar 14, 2018

jgiardino mentioned this pull request Apr 11, 2018

Documentation for structural patterns #303

Closed

mturley force-pushed the modal-wizard branch 2 times, most recently from 83fa55c to a2c1f5b Compare April 15, 2018 19:04

mturley changed the title ~~feat(WizardPattern): Add WizardPattern and StatefulWizardPattern components~~ BREAKING CHANGE: feat(WizardPattern): Add WizardPattern and StatefulWizardPattern components Apr 16, 2018

mturley force-pushed the modal-wizard branch from a2c1f5b to a3429fe Compare April 16, 2018 14:20

mturley changed the title ~~BREAKING CHANGE: feat(WizardPattern): Add WizardPattern and StatefulWizardPattern components~~ feat(WizardPattern): Add WizardPattern and StatefulWizardPattern components Apr 16, 2018

mturley force-pushed the modal-wizard branch from a3429fe to 4f93d55 Compare April 16, 2018 14:21

priley86 previously approved these changes Apr 16, 2018

View reviewed changes

mturley dismissed priley86’s stale review via 6ead9a0 April 17, 2018 15:43

mturley force-pushed the modal-wizard branch from 4f93d55 to 6ead9a0 Compare April 17, 2018 15:43

jeff-phillips-18 previously approved these changes Apr 17, 2018

View reviewed changes

feat(WizardPattern): Clean up Wizard, Add WizardPattern and StatefulW…

446d574

…izardPattern components BREAKING CHANGE: Wizard structure has changed

mturley dismissed jeff-phillips-18’s stale review via 446d574 April 17, 2018 22:14

mturley force-pushed the modal-wizard branch from 6ead9a0 to 446d574 Compare April 17, 2018 22:14

priley86 approved these changes Apr 18, 2018

View reviewed changes

jeff-phillips-18 approved these changes Apr 18, 2018

View reviewed changes

jeff-phillips-18 merged commit 2e2ffed into patternfly:master Apr 18, 2018

vojtechszocs mentioned this pull request Sep 7, 2018

[WIP] Implement Create VM Wizard component kubevirt/web-ui-components#4

Closed

priley86 added the Breaking change 💥 this change requires a major release and has API changes. label Oct 2, 2018

feat(WizardPattern): Add WizardPattern and StatefulWizardPattern components #268

feat(WizardPattern): Add WizardPattern and StatefulWizardPattern components #268

Uh oh!

Conversation

mturley commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1207

💛 - Coveralls

Uh oh!

priley86 commented Mar 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mturley Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mturley Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mturley Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mturley Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdcabrera commented Mar 13, 2018

Uh oh!

mturley commented Mar 13, 2018

Uh oh!

mturley commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mturley commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mturley commented Mar 13, 2018

Uh oh!

mturley commented Mar 13, 2018

Uh oh!

priley86 commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vojtechszocs commented Mar 14, 2018

Uh oh!

mturley commented Mar 12, 2018 •

edited

Loading

coveralls commented Mar 12, 2018 •

edited

Loading

mturley Mar 15, 2018 •

edited

Loading

mturley Mar 15, 2018 •

edited

Loading

priley86 commented Mar 12, 2018 •

edited

Loading

mturley Mar 13, 2018 •

edited

Loading

mturley Mar 13, 2018 •

edited

Loading

mturley commented Mar 13, 2018 •

edited

Loading

mturley commented Mar 13, 2018 •

edited

Loading

priley86 commented Mar 14, 2018 •

edited

Loading

priley86 commented Mar 14, 2018 •

edited

Loading

mturley commented Mar 15, 2018 •

edited

Loading

priley86 commented Apr 5, 2018 •

edited

Loading