fix:(alert): updates alert - #1208
Merged
Merged
Conversation
Collaborator
|
Deploy preview for pf-next ready! Built with commit 470d129 |
mcoker
reviewed
Jan 10, 2019
mcoker
reviewed
Jan 10, 2019
| --pf-c-alert__title--Color: var(--pf-global--Color--100); | ||
| --pf-c-alert__title__siblings--MarginTop: var(--pf-global--spacer--xs); |
Contributor
There was a problem hiding this comment.
I would probably use --pf-c-alert__title--siblings--MarginTop since __ typically refers to a BEM element, but I don't feel strongly about it.
We do something similar with --nested
$ grep -ri 'nested--' src/
src//patternfly/components/Content/content.scss: --pf-c-content--ul--nested--MarginTop: var(--pf-global--spacer--sm);
src//patternfly/components/Content/content.scss: --pf-c-content--ul--nested--MarginLeft: var(--pf-global--spacer--sm);
src//patternfly/components/Content/content.scss: margin-top: var(--pf-c-content--ul--nested--MarginTop);
src//patternfly/components/Content/content.scss: margin-left: var(--pf-c-content--ul--nested--MarginLeft);
src//patternfly/components/List/list.scss: --pf-c-content--ul--nested--MarginTop: var(--pf-global--spacer--sm);
src//patternfly/components/List/list.scss: --pf-c-content--ul--nested--MarginLeft: var(--pf-global--spacer--sm);
src//patternfly/components/List/list.scss: margin-top: var(--pf-c-content--ul--nested--MarginTop);
src//patternfly/components/List/list.scss: margin-left: var(--pf-c-content--ul--nested--MarginLeft);
Contributor
Author
There was a problem hiding this comment.
ya I was the same. I'm cool with --
Contributor
|
Would we ever have both a close icon and an action button in an alert? |
Contributor
Author
Not according to @mcarrano. We do have them in the alert migration demo so I've created something there. But I'd guess that we'd move that text to be a text link rather than a button beside the dismiss. |
matthewcarleton
force-pushed
the
alert-updates-is1099
branch
from
January 11, 2019 13:16
b2584e3 to
b213b57
Compare
matthewcarleton
force-pushed
the
alert-updates-is1099
branch
from
January 11, 2019 17:01
b213b57 to
847e4cf
Compare
mcoker
reviewed
Jan 11, 2019
| @@ -0,0 +1,6 @@ | |||
| <div class="pf-c-alert__dismiss{{#if alert-dismiss--modifier}} {{alert-dismiss--modifier}}{{/if}}" | |||
Contributor
There was a problem hiding this comment.
I think you can remove this file now
mcoker
reviewed
Jan 11, 2019
| @@ -2,6 +2,9 @@ | |||
| {{#> alert-icon alert-icon--success="true"}} | |||
| {{/alert-icon}} | |||
| {{#> alert-body alert-body--attribute='role="alert"'}} | |||
| {{#> button button--modifier="pf-m-plain" button--attribute='aria-label="Remove"'}} | |||
Contributor
There was a problem hiding this comment.
should the aria-label read "Close"?
Contributor
Author
There was a problem hiding this comment.
oh hmm...maybe? Or maybe dismiss?
matthewcarleton
force-pushed
the
alert-updates-is1099
branch
from
January 11, 2019 20:22
084c9d0 to
dbddbcb
Compare
mcoker
reviewed
Jan 11, 2019
| {{/alert-body}} | ||
| {{/alert}} | ||
| <br> | ||
| {{#> alert alert--modifier="pf-m-success" alert--attribute='aria-label="success Notification"'}} |
Contributor
There was a problem hiding this comment.
nitpick, can you capitalize "success Notification"?
matthewcarleton
force-pushed
the
alert-updates-is1099
branch
from
January 11, 2019 21:13
dbddbcb to
619e5eb
Compare
matthewcarleton
force-pushed
the
alert-updates-is1099
branch
from
January 11, 2019 21:31
619e5eb to
470d129
Compare
This was referenced Jan 14, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This closes #1099