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(modal): add left aligned footer buttons variation #2182

Merged
merged 3 commits into from Aug 30, 2019

Conversation

mcoker
Copy link
Contributor

@mcoker mcoker commented Aug 22, 2019

fixes #1599

@patternfly-build
Copy link

PatternFly-Next preview: https://patternfly-next-pr-2182.surge.sh

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@mcoker I see that you've moved the buttons to the left, but the order also needs to change. The submit (Save) button should come first, i.e. Save, Cancel.

Also something seems to be messed up with the examples as I see the same thing for each example. I assume you are keeping around the right aligned examples as not to break anything, but can you reorganize the workspace to place the left-aligned example first as the default placement? The way it is now, it feels like this is a secondary variant as opposed to the preferred approach.

Copy link
Contributor

@mattnolting mattnolting left a comment

{{#> button button--modifier="pf-m-primary"}}
Save
{{/button}}
{{/modal-box-footer}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should Save come first, before Cancel?

@mattnolting
Copy link
Contributor

@redallen Actually, none of the demo links are working as expected..

@mcoker
Copy link
Contributor Author

mcoker commented Aug 23, 2019

@mcarrano

submit (Save) button should come first, i.e. Save, Cancel.

D'oh! Updated.

Also something seems to be messed up with the examples as I see the same thing for each example. I assume you are keeping around the right aligned examples as not to break anything, but can you reorganize the workspace to place the left-aligned example first as the default placement? The way it is now, it feels like this is a secondary variant as opposed to the preferred approach.

Currently the left-aligned buttons is just a variation, and not the default, so I listed it as we would any other variation from the default. But I see your point. Do we even want to show an example with right aligned buttons, or should they all simply be updated to be left aligned? Or do we want them all to be left aligned, then we show a new example that illustrates the old/legacy alignment where they're right aligned?

@mcarrano
Copy link
Member

@mcoker I would vote for only showing left aligned examples, otherwise it feels like we are saying that you have the option to do either. @LHinson do you agree with that?

@mcoker
Copy link
Contributor Author

mcoker commented Aug 28, 2019

@mcarrano @mattnolting updated the examples so they all use the left aligned modifier, and added right aligned demo

@mcarrano
Copy link
Member

Seem to be having a problem with the preview @mcoker . All examples are layering to top of each other.

Screen Shot 2019-08-29 at 10 24 17 AM

@mcoker
Copy link
Contributor Author

mcoker commented Aug 29, 2019

@rachael-phillips see @mcarrano's comment ^. That's the workspace problem we discussed yesterday.

@mcarrano there's a bug in the preview where all of the examples for a component/demo all show up on one page, so since these demos all occupy the entire viewport width/height, they're overlapped on top of one another. Here are screenshots of the demos:

Modal demo

Screen Shot 2019-08-29 at 4 16 18 PM

Modal demo - content long enough to scroll

Screen Shot 2019-08-29 at 4 16 20 PM

Modal demo - large

Screen Shot 2019-08-29 at 4 16 23 PM

Modal demo - legacy footer button alignment (right aligned)

Screen Shot 2019-08-29 at 4 16 25 PM

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

Other than the preview issue I mentioned, this looks good @mcoker

Copy link
Contributor

@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

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

LGTM!

@mattnolting mattnolting merged commit dbe25cc into patternfly:master Aug 30, 2019
@redallen
Copy link
Contributor

🎉 This PR is included in version 2.27.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mcoker mcoker deleted the issue-1599 branch December 16, 2019 22:44
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.

None yet

5 participants