feat(Wizard): add support for in page rendering by atiratree · Pull Request #2830 · patternfly/patternfly-react · GitHub

fixes #2751

The css require at least "@patternfly/patternfly": "2.28.0" dependency but only has "@patternfly/patternfly": "2.27.3". Should it be resolved in this PR?

title is now optional, because the header is not rendered in in-page wizard

The default wizard requires a title currently, and per conversations with @mcarrano here (patternfly/patternfly#2191) I believe we want to keep it that way. Making it optional creates these 2 scenarios:

Title/description is undefined renders an empty header, and the header is required at a minimum to position the close button.

Title is undefined but a description exists, which seems to work OK, but we haven't really accounted for that in core and I'm not sure we want to allow that per the design:

Can we make this optional if isInPage is true? And technically it isn't optional - the entire header shouldn't render if using an in-page wizard, so title and description shouldn't be allowed.

We can change the title to optional here since it technically is not always required. However, can you generate a warning if the title is not provided AND isModal is true.

user should not have an ability to change the inpage wizard to modal one after it is constructed

not sure if this is needed, but the titleId/descriptionId are not used when in-page

I guess we need to update @patternfly/patternfly" dependency for the tests to pass (fails on missing inPage modifier ).

should I do it in this PR?

There's a PR up to bump the version to 2.28 so when that merges in it should resolve your issues.

@suomiy Version bump PR has been merged, try updating and that should solve the build error!

PatternFly-React preview: https://patternfly-react-pr-2830.surge.sh

updated & tests passed

Looks good

@suomiy this looks awesome, thanks for doing this so quickly!

Looks great @suomiy! One bit of feedback about the title/description.

The default wizard requires a title currently, and per conversations with @mcarrano here (patternfly/patternfly#2191) I believe we want to keep it that way. Making it optional creates these 2 scenarios:

Title/description is undefined renders an empty header, and the header is required at a minimum to position the close button.

Title is undefined but a description exists, which seems to work OK, but we haven't really accounted for that in core and I'm not sure we want to allow that per the design:

Can we make this optional if isInPage is true? And technically it isn't optional - the entire header shouldn't render if using an in-page wizard, so title and description shouldn't be allowed.

Can we make this optional if isInPage is true? And technically it isn't optional - the entire header shouldn't render if using an in-page wizard, so title and description shouldn't be allowed.

@mcoker

These two are mutually exclusive from the type perspective. I.e. having title optional in page and required in modal.

Our options are:

Quick Patch

leave it required. Users will be force to specify the title and this will have no effects
leave it optional. Users will have mentioned positioning problems if they do not specify it

Fix Patch

pass WizardHeader component to the wizard (breaking changes)
fix css

leave it required. Users will be force to specify the title and this will have no effects
leave it optional. Users will have mentioned positioning problems if they do not specify it

I'm not sure between the two, what do you think @tlabaj?

We can change the title to optional here since it technically is not always required. However, can you generate a warning if the title is not provided AND isModal is true.

Alternative workaround to optional title, in WizardHeader render empty non-breaking space if not provided

{title ? (
        <Title
          size="3xl"
          className={css(styles.wizardTitle)}
          aria-label={title}
          id={titleId}
        >
          {title}
        </Title>
      ) : (
        <Title
          size="3xl"
          className={css(styles.wizardTitle)}
          aria-label={title}
          id={titleId}
        >
          &nbsp;
        </Title>
      )}

LGTM. We can take this in and update the docs to let the user know that they should pass in a title for the modal

I decided to go the   route. Should I still add the warning?

Also, should I create an issue for that (to fix it properly and remove the non breaking space)?

@suomiy that would be great thanks. can you just also update the prop documentation for the title prop to say that it is required unless isInPage is set to true?

added the warning and the documentation

awesome, thanks for making these updates @suomiy

LGTM

@suomiy looks great! Thanks for all your hard work on this!!

Your changes have been released in:

@patternfly/react-core@3.97.0
@patternfly/react-docs@4.11.1
@patternfly/react-inline-edit-extension@2.11.19
demo-app-ts@2.21.22
@patternfly/react-table@2.19.19
@patternfly/react-topology@2.8.18
@patternfly/react-virtualized-extension@1.2.7

Thanks for your contribution! 🎉

nice! 🎉

atiratree commented Sep 4, 2019

View reviewed changes

atiratree force-pushed the inpageWizard branch from 0bb3b25 to fd57817 Compare September 4, 2019 15:23

tlabaj requested a review from jschuler September 4, 2019 18:46

tlabaj assigned jschuler Sep 4, 2019

tlabaj added css review labels Sep 4, 2019

tlabaj requested review from dlabaj, mcarrano and mcoker September 4, 2019 18:47

atiratree force-pushed the inpageWizard branch from fd57817 to 3a56593 Compare September 4, 2019 20:01

mcarrano previously approved these changes Sep 4, 2019

View reviewed changes

mcarrano added ux approved and removed ux review labels Sep 4, 2019

matthewcarleton previously approved these changes Sep 5, 2019

View reviewed changes

kmcfaul previously approved these changes Sep 5, 2019

View reviewed changes

jschuler reviewed Sep 5, 2019

View reviewed changes

Comment thread packages/patternfly-4/react-core/src/components/Wizard/Wizard.md Outdated

Comment thread packages/patternfly-4/react-core/src/components/Wizard/Wizard.tsx Outdated

mcoker requested changes Sep 5, 2019

View reviewed changes

atiratree dismissed stale reviews from kmcfaul, matthewcarleton, and mcarrano via cd30a39 September 5, 2019 14:43

atiratree force-pushed the inpageWizard branch from 3a56593 to cd30a39 Compare September 5, 2019 14:43

tlabaj suggested changes Sep 5, 2019

View reviewed changes

jschuler reviewed Sep 5, 2019

View reviewed changes

Comment thread packages/patternfly-4/react-core/src/components/Wizard/Wizard.tsx

jschuler previously approved these changes Sep 5, 2019

View reviewed changes

atiratree dismissed jschuler’s stale review via 8d008dd September 5, 2019 19:20

atiratree force-pushed the inpageWizard branch from cd30a39 to 8d008dd Compare September 5, 2019 19:20

feat(Wizard): add support for in page rendering

911acbf

atiratree force-pushed the inpageWizard branch from 8d008dd to 911acbf Compare September 5, 2019 19:33

jschuler approved these changes Sep 5, 2019

View reviewed changes

tlabaj approved these changes Sep 5, 2019

View reviewed changes

kmcfaul approved these changes Sep 5, 2019

View reviewed changes

mcoker approved these changes Sep 5, 2019

View reviewed changes

jschuler merged commit bf96474 into patternfly:master Sep 5, 2019

                 height?: number | string;
                 /** The wizard title */
-                title: string;
+                title?: string;

                   const newId = Wizard.currentId++;
-                  this.titleId = `pf-wizard-title-${newId}`;
-                  this.descriptionId = `pf-wizard-description-${newId}`;
+                  this.isModal = !props.isInPage;

                 height?: number | string;
                 /** The wizard title */
-                title: string;
+                title?: string;

                 height?: number | string;
                 /** The wizard title */
-                title: string;
+                title?: string;

feat(Wizard): add support for in page rendering by atiratree · Pull Request #2830 · patternfly/patternfly-react · GitHub

Uh oh!

feat(Wizard): add support for in page rendering - #2830

feat(Wizard): add support for in page rendering#2830jschuler merged 1 commit intopatternfly:masterpatternfly/patternfly-react:masterfrom atiratree:inpageWizardatiratree/patternfly-react:inpageWizardCopy head branch name to clipboard

Conversation

atiratree commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atiratree commented Sep 4, 2019

Uh oh!

Uh oh!

atiratree Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

mcoker Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

tlabaj Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atiratree Sep 4, 2019 • 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!

atiratree Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

atiratree commented Sep 4, 2019

Uh oh!

kmcfaul commented Sep 4, 2019

Uh oh!

kmcfaul commented Sep 4, 2019

Uh oh!

patternfly-build commented Sep 4, 2019

Uh oh!

atiratree commented Sep 4, 2019

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

matthewcarleton commented Sep 5, 2019

Uh oh!

Uh oh!

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

atiratree commented Sep 5, 2019

Uh oh!

mcoker commented Sep 5, 2019

Uh oh!

tlabaj Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jschuler commented Sep 5, 2019

Uh oh!

jschuler left a comment

Choose a reason for hiding this comment

Uh oh!

atiratree commented Sep 5, 2019

Uh oh!

atiratree commented Sep 5, 2019

Uh oh!

jschuler commented Sep 5, 2019

Uh oh!

atiratree commented Sep 5, 2019

Uh oh!

jschuler left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

feat(Wizard): add support for in page rendering#2830
jschuler merged 1 commit into
patternfly:masterfrom
atiratree:inpageWizard

atiratree commented Sep 4, 2019 •
edited
Loading

atiratree Sep 4, 2019 •
edited
Loading