fix(modalbox): add the small modifier to modalbox

[boaz0]

What:

closes #1291 and #1138

//cc @mcoker can you take a look to make sure I didn't miss anything. Thanks.

[coveralls]

Pull Request Test Coverage Report for Build 4578

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• 31 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-1.02%) to 77.858%

---

Files with Coverage Reduction	New Missed Lines	%
packages/patternfly-4/react-core/src/components/Dropdown/Separator.js	1	80.0%
packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js	4	34.52%
packages/patternfly-4/react-core/src/helpers/util.ts	26	14.29%

Totals
Change from base Build 4568:	-1.02%
Covered Lines:	658
Relevant Lines:	787

---

💛 - Coveralls

[mcoker]

thanks @boaz0, looks like the preview isn't working so I can't verify visually. though looking at the code, I'm not sure if it should be part of this PR or not, but this change was also made to enable users to pass a width prop to the modal component with a custom width that would be used without either isSmall (.pf-m-sm) or isLarge (.pf-m-lg) per the issue you opened #1138. Per that issue, now if you want a full width modal, you would just omit isSmall and isLarge, because the modal will now be 100% width by default.

[patternfly-build]

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

[boaz0]

@mcoker fixed that: https://1372-pr-patternfly-react-patternfly.surge.sh/patternfly-4/components/modal

[dlabaj]

Can you add an example on how to change the width to a user defined width (e.g. 400px)? Also I think we need a width prop.

[mcoker]

Looks like there is an extra div between div.pf-l-bullseye and div.pf-c-modal-box that is causing an alignment issue. Is it possible to get rid of that or will that always be there with this component? If it will always be there, I'll need to make a small adjustment in core.

[boaz0]

@mcoker unfortunately I can't remove this. This is one of the "side-effects" of FocusTrap to catch clicks outside the modal.

[boaz0]

@dlabaj I fixed that. Thanks for your review.

[redallen]

Your Travis build is likely fine, just merge master and push to get the latest CI changes.

[boaz0]

thanks @redallen - did git rebase.

[tlabaj]

@boaz0 Can you please rebase again please. Some changes were made in Master to fix failing Build. Thanks.

[mcoker]

@boaz0 gotcha. Is there any way to add a class or otherwise style the generated element? If not we're going to need to reconsider components we currently center using the pf-l-bullseye layout.

[boaz0]

@mcoker looking at the documents, <FocusTrap> gets three props: active, pasued and focursTrapOptions .
None of those provide a way to set className or define the element that is wrapped over the modal.

So I did what you suggested and used Bullseye to center the modal.

[mcoker]

Gotcha, thanks. I don't think it's ideal to have to use 2 Bullseye's to center something like that. We have a CSS meeting tomorrow, and I'll add this to our agenda of things to discuss.

[mcoker]

@boaz0 for now I think what you've done with 2 Bullseyes is fine.

is it common for a component like <FocusTrap> to allow for a ClassName? I'm curious if we can request this be added, so we could add the .pf-l-bullseye class to it?

[mcoker]

thanks!!

[boaz0]

Opened a ticket focus-trap/focus-trap-react#40 for that thing.

I will update you when I will have something.

[boaz0]

Hi @mcoker I have good news. I took a look at the source code and it seems that I was wrong.
You can specify props like id, className or any HTML props and it will be attached to the div element.

I am updating the code and tests and pushing it for your review.
Hope I didn't make you work too hard on this.

Thanks.

[mcoker]

@boaz0 awesome! Good to know, I'm sure we'll use FocusTrap in other places and will need that, too.

[jschuler]

Should we add clarification, something like Use either the width or the isLarge/isSmall prop

[boaz0]

I will check this out.
Although if I am not wrong isLarge and isSmall are setting maxWidth and minWidth to Modal respectively.

Thanks.

[boaz0]

Awesome! 🙌 Thank you!