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-box): added support for title icon, modal alert states #3487

Merged
merged 3 commits into from
Sep 30, 2020

Conversation

mcoker
Copy link
Contributor

@mcoker mcoker commented Sep 15, 2020

fixes #3374

@patternfly-build
Copy link

patternfly-build commented Sep 15, 2020

Preview: https://patternfly-pr-3487.surge.sh

A11y report: https://patternfly-pr-3487-coverage.surge.sh

CSS Size Report
NameCurrentPreviousDiff %
components/ModalBox/modal-box.css7.7 kB6.3 kB17.69
patternfly-no-reset.css866.7 kB865.3 kB0.16
patternfly.css868.6 kB867.2 kB0.16
patternfly.min.css768.4 kB767.1 kB0.16

Copy link

@mmenestr mmenestr left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Looks good @mcoker But maybe change the title of the last example to read "Error alert modal title" for consistency with the others?

@mcoker
Copy link
Contributor Author

mcoker commented Sep 16, 2020

@mcarrano updated!

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.

Looks great. Thanks @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.

The spacing between the icon and title text looks a bit tight to me, but implementation is sound.

@mmenestr
Copy link

Yes, good catch @mattnolting. I initially had it mocked up as 16px, not 8px (as it appears to be but not sure).

--pf-c-modal-box__title-icon--Color: var(--pf-c-modal-box--m-success__title-icon--Color);
}

&.pf-m-default {
Copy link
Member

Choose a reason for hiding this comment

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

do you need a modifier for default variation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so. Without a modifier, the icon is just a regular text color icon. With an alert modifier, it represents the alert state, and "default" is an alert state. WDYT?

<i class="fas fa-info-circle" aria-hidden="true"></i>
{{else if modal-box--IsSuccessAlert}}
<i class="fas fa-check-circle" aria-hidden="true"></i>
{{else if modal-box--IsWarningAlert}}
Copy link
Member

Choose a reason for hiding this comment

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

should this use fa-fw so all the icons line up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated!

Copy link
Member

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

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

LPTM

@mattnolting mattnolting merged commit 5239f62 into patternfly:master Sep 30, 2020
@redallen
Copy link
Contributor

🎉 This PR is included in version 4.47.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Introduce modal alert variation
7 participants