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

fix(Wizard): set a custom width to the wizard modal #5256

Merged
merged 2 commits into from
Jan 15, 2021
Merged

fix(Wizard): set a custom width to the wizard modal #5256

merged 2 commits into from
Jan 15, 2021

Conversation

boaz0
Copy link
Member

@boaz0 boaz0 commented Dec 14, 2020

What: Closes #5254

//cc @MariaAga

@patternfly-build
Copy link
Contributor

patternfly-build commented Dec 14, 2020

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@MariaAga
Copy link
Contributor

@boaz0 Thanks for the quick fix! It looks like this only changes the width of the wizard if it's in a modal. But a user still can't set custom width for the wizard like they can set the height.

@boaz0
Copy link
Member Author

boaz0 commented Dec 14, 2020

Um, sorry I didn't realized that.

Can you elaborate more what is the use case? If you have mocks or screenshots that will be helpful too.

Thanks.

@MariaAga
Copy link
Contributor

I saw that there were a width prop that wasn't used so it was weird.
And about my use case, I'm not sure if the solution it max-width prop or width prop but my wizard looks a bit funny being so long :)
Screenshot from 2020-12-14 11-41-10
Screenshot from 2020-12-14 11-41-04

@boaz0
Copy link
Member Author

boaz0 commented Dec 14, 2020

Uh huh. I see.
I don't think that is what width was supposed to do.
Nonetheless, I will add this to the PR and let @tlabaj @jschuler & @redallen to review it as well.

Anyway, what I do to fix this is using the flex layout or set width to 360px in the Form's style.

Thanks.

@codecov-io
Copy link

Codecov Report

Merging #5256 (a80fe41) into master (337a667) will decrease coverage by 0.00%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5256      +/-   ##
==========================================
- Coverage   52.20%   52.19%   -0.01%     
==========================================
  Files         575      575              
  Lines       10972    10976       +4     
  Branches     4100     4103       +3     
==========================================
+ Hits         5728     5729       +1     
  Misses       4513     4513              
- Partials      731      734       +3     
Flag Coverage Δ
patternfly4 52.19% <20.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ckages/react-core/src/components/Wizard/Wizard.tsx 45.45% <20.00%> (-0.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13dea26...a80fe41. Read the comment docs.

const wizard = (
<WizardContextProvider value={context}>
<div
{...rest}
className={css(styles.wizard, activeStep && activeStep.isFinishedStep && 'pf-m-finished', className)}
{...(height && { style: { height } })}
style={Object.keys(divStyles).length ? divStyles : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have CSS vars you can use here or via setProperty() to control the width and max-width. That's usually preferred as we may use that variable to help calculate some other property value that references the modal's width.

--pf-c-modal-box--Width - sets the width
--pf-c-modal-box--MaxWidth - sets the max-width

Helpful if you want the modal to be say, 50%, but not exceed 800px (--pf-c-modal-box--Width: 50%; --pf-c-modal-box--MaxWidth: 800px;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it apply on the wizard when it's not displayed inside a modal?

Copy link
Contributor

@mcoker mcoker Dec 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry @boaz0, I read this wrong, I thought the width applied to the modal. My suggestion would not change anything with the wizard at all. It should only change the modal. When the wizard is used in a modal, it is set to fill the full width and height of the modal. So by updating the modal's dimensions, the wizards dimensions automatically change to match the modal's.

If this feature is only to change the width of the wizard when in a modal, personally I think we should change the width of the modal. And instead of using the var I suggested, I just looked up the modal docs and <Modal> supports a width prop.

But if we also want the ability for someone to pass a width to the wizard when used outside of a modal, the approach in the PR makes sense. Another approach we could take for that, assuming the wizard is used in a page section, is to allow a custom max-width on page sections, and allow the user to set the max width on the page section the wizard is in.

Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...if we also want the ability for someone to pass a width to the wizard when used outside of a modal, the approach in the PR makes sense.

@MariaAga Is this what you are asking for?

@MariaAga
Copy link
Contributor

MariaAga commented Jan 4, 2021

@redallen Yes as I saw that in the Wizard documentation here: https://www.patternfly.org/v4/components/wizard#props

width | number | string | No | null | Custom width of the wizard

But as mcoker said I can use --pf-c-modal-box--Width for that (or max-width).

Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see the issue -- we left behind an unused width prop that used to be for the modal but should now be for the Wizard itself.

This change LGTM although I'd like to deprecate the width and height props in favor of passing style={{ width: '', height: '' }}.

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @boaz0!

@tlabaj tlabaj merged commit 22dd8f6 into patternfly:master Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Width prop is not used in the Wizard component
8 participants