Skip to content
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 support for in page rendering #2830

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@suomiy
Copy link
Contributor

suomiy commented Sep 4, 2019

fixes #2751

inpage

@suomiy

This comment has been minimized.

Copy link
Contributor Author

suomiy commented Sep 4, 2019

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?

@@ -56,11 +58,11 @@ export interface WizardProps extends React.HTMLProps<HTMLDivElement> {
/** Custom height of the wizard */
height?: number | string;
/** The wizard title */
title: string;
title?: string;

This comment has been minimized.

Copy link
@suomiy

suomiy Sep 4, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@mcoker

mcoker Sep 5, 2019

Contributor

The default wizard requires a title currently, and per conversations with @mcarrano here (patternfly/patternfly-next#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.

Screen Shot 2019-09-05 at 9 18 11 AM

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:

Screen Shot 2019-09-05 at 9 21 06 AM

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.

This comment has been minimized.

Copy link
@tlabaj

tlabaj Sep 5, 2019

Contributor

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.


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

This comment has been minimized.

Copy link
@suomiy

suomiy Sep 4, 2019

Author Contributor

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

if (this.isModal) {
this.titleId = `pf-wizard-title-${newId}`;
this.descriptionId = `pf-wizard-description-${newId}`;
}

This comment has been minimized.

Copy link
@suomiy

suomiy Sep 4, 2019

Author Contributor

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

@suomiy

This comment has been minimized.

Copy link
Contributor Author

suomiy commented Sep 4, 2019

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?

@suomiy suomiy force-pushed the suomiy:inpageWizard branch from 0bb3b25 to fd57817 Sep 4, 2019
@kmcfaul

This comment has been minimized.

Copy link
Contributor

kmcfaul commented Sep 4, 2019

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

@kmcfaul

This comment has been minimized.

Copy link
Contributor

kmcfaul commented Sep 4, 2019

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

@suomiy suomiy force-pushed the suomiy:inpageWizard branch from fd57817 to 3a56593 Sep 4, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 4, 2019

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

@suomiy

This comment has been minimized.

Copy link
Contributor Author

suomiy commented Sep 4, 2019

updated & tests passed

Copy link
Member

mcarrano left a comment

Looks good

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

This comment has been minimized.

Copy link

matthewcarleton commented Sep 5, 2019

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

Copy link
Contributor

mcoker left a comment

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

@@ -56,11 +58,11 @@ export interface WizardProps extends React.HTMLProps<HTMLDivElement> {
/** Custom height of the wizard */
height?: number | string;
/** The wizard title */
title: string;
title?: string;

This comment has been minimized.

Copy link
@mcoker

mcoker Sep 5, 2019

Contributor

The default wizard requires a title currently, and per conversations with @mcarrano here (patternfly/patternfly-next#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.

Screen Shot 2019-09-05 at 9 18 11 AM

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:

Screen Shot 2019-09-05 at 9 21 06 AM

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.

@suomiy suomiy dismissed stale reviews from kmcfaul, matthewcarleton, and mcarrano via cd30a39 Sep 5, 2019
@suomiy suomiy force-pushed the suomiy:inpageWizard branch from 3a56593 to cd30a39 Sep 5, 2019
@suomiy

This comment has been minimized.

Copy link
Contributor Author

suomiy commented Sep 5, 2019

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
@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Sep 5, 2019

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?

@@ -56,11 +58,11 @@ export interface WizardProps extends React.HTMLProps<HTMLDivElement> {
/** Custom height of the wizard */
height?: number | string;
/** The wizard title */
title: string;
title?: string;

This comment has been minimized.

Copy link
@tlabaj

tlabaj Sep 5, 2019

Contributor

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.

@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 5, 2019

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>
      )}
Copy link
Collaborator

jschuler left a comment

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

@suomiy suomiy force-pushed the suomiy:inpageWizard branch from cd30a39 to 8d008dd Sep 5, 2019
@suomiy

This comment has been minimized.

Copy link
Contributor Author

suomiy commented Sep 5, 2019

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

@suomiy

This comment has been minimized.

Copy link
Contributor Author

suomiy commented Sep 5, 2019

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

@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 5, 2019

@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?

@suomiy suomiy force-pushed the suomiy:inpageWizard branch from 8d008dd to 911acbf Sep 5, 2019
@suomiy

This comment has been minimized.

Copy link
Contributor Author

suomiy commented Sep 5, 2019

added the warning and the documentation

Copy link
Collaborator

jschuler left a comment

awesome, thanks for making these updates @suomiy

@tlabaj
tlabaj approved these changes Sep 5, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@kmcfaul
kmcfaul approved these changes Sep 5, 2019
@mcoker
mcoker approved these changes Sep 5, 2019
Copy link
Contributor

mcoker left a comment

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

@jschuler jschuler merged commit bf96474 into patternfly:master Sep 5, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 5, 2019

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! 🎉

@suomiy

This comment has been minimized.

Copy link
Contributor Author

suomiy commented Sep 5, 2019

nice! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.