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(Modal): add modifier to left align Modal footer #2835

Merged
merged 3 commits into from Sep 6, 2019

Conversation

@tlabaj
Copy link
Contributor

tlabaj commented Sep 4, 2019

#2744

What:

Additional issues:

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 4, 2019

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

Copy link
Member

mcarrano left a comment

Looks like you slid the buttons over @tlabaj but did not swap the order. The primary (submit) button should always come first, i.e. be on the left.

Copy link
Contributor

mcoker left a comment

LGTM! Buttons just need to be re-arranged per @mcarrano's comment.

Titani
@tlabaj tlabaj dismissed stale reviews from mcoker and kmcfaul via 4c76090 Sep 5, 2019
Copy link
Member

mcarrano left a comment

This looks good now @tlabaj . I just have one question to float out there. This example looks odd to me now:

Screen Shot 2019-09-05 at 12 12 26 PM

I'm wondering if we should just remove that and have a separate component or variation for something like this with a centered button (which is what I would expect). I don't see this example in core. @mcoker @kybaker @mceledonia what do you think? If I'm the only one bothered by this, I'm ok to leave it alone.

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Sep 5, 2019

@mcarrano hmmm. We do have this example in core, but we show how to use the existing modal close button without a header - https://pf4.patternfly.org/components/ModalBox/examples/

Screen Shot 2019-09-05 at 12 07 14 PM

Personally I think we should update the react example to match, assuming that's the preferred design approach for a modal without a header (to use the existing close button versus a custom one), since we don't have an example for that in react currently.

Then I think we could discuss if we need to add another example that removes the modal's close button and demonstrates how to use a custom button to close the modal. If we do provide that example, I guess how we present that would depend on whether we want to either officially support or recommend a design for that use case. If we support it, we could add a variant to the footer that positions the custom close button. If we just recommend a design, maybe we use the flex layout as a demo of how we suggest doing it. Or do we just leave it as is, demonstrating how to add the button, but not necessarily recommending a design for the button's placement.

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Sep 5, 2019

In the context of this PR, I like your suggestion @mcoker . @tlabaj can we just update the 'Modal with no header' example to match core?

I think we could take up the question of whether we want an explicit Close button as a separate design issue.

Titani
Copy link
Member

mcarrano left a comment

Looks great. Thanks for making this change @tlabaj !

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

mcoker left a comment

LGTM! 🎉

@kmcfaul
kmcfaul approved these changes Sep 5, 2019
@dlabaj
dlabaj approved these changes Sep 6, 2019
@dlabaj dlabaj merged commit e6c7694 into patternfly:master Sep 6, 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 6, 2019

Your changes have been released in:

  • @patternfly/react-core@3.99.0
  • @patternfly/react-docs@4.11.3
  • @patternfly/react-inline-edit-extension@2.11.21
  • demo-app-ts@2.22.0
  • @patternfly/react-integration@2.22.0
  • @patternfly/react-table@2.20.1
  • @patternfly/react-topology@2.8.20
  • @patternfly/react-virtualized-extension@1.2.9

Thanks for your contribution! 🎉

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.