feat(alert): adds inline modifier by srambach · Pull Request #1775 · patternfly/patternfly

srambach · 2019-04-29T17:45:26Z

Based on design doc at https://docs.google.com/document/d/1HTkvK8_zFJAX-rhBw1aNrmZYzywixik31DzaiKCUbW4/edit#

Fixes #1390

Note: The icon padding should be broken into parts but is a breaking change so there is an issue for this in #1773

patternfly-build · 2019-04-29T17:49:07Z

Deploy preview for pf-next ready!

Built with commit a6651af

https://deploy-preview-1775--pf-next.netlify.com

bdellasc · 2019-04-29T18:03:11Z

Looks good to me @srambach - thanks!

mcoker · 2019-04-29T18:11:42Z

src/patternfly/components/Alert/alert.scss

+  --pf-c-alert__m-inline__icon--PaddingRight: 0;
+  --pf-c-alert__m-inline__icon--PaddingBottom: var(--pf-global--spacer--md);
+  --pf-c-alert__m-inline__icon--PaddingLeft: var(--pf-global--spacer--md);
+  --pf-c-alert--m-success-inline__icon--Color: var(--pf-global--success-color--100);


Can you separate these vars out into --m-[state]--m-inline, so not to confuse it as ineferring .pf-m-[state]-inline?

mcoker · 2019-04-29T18:16:44Z

src/patternfly/components/Alert/alert.scss

+    border-color: var(--pf-c-alert--m-inline--BorderColor);
+    border-style: var(--pf-c-alert--m-inline--BorderStyle);
+    border-width: var(--pf-c-alert--m-inline--BorderWidth);
+    border-left-color: var(--pf-c-alert--m-inline--BorderLeftColor);


You can write this in shorthand to save some lines. We also typically don't define border-style as a var, but you can if you want.

That said how about we define this border as a pseudo element instead? The triangle corner is somewhat obvious.

mcoker

awesome! we've needed this for a while. left a few comments for your review.

bdellasc · 2019-04-30T14:49:57Z

The latest changes look great, @srambach - thank you!

mcoker · 2019-04-30T15:19:46Z

src/patternfly/components/Alert/alert.scss

+    border-top: var(--pf-c-alert--m-inline--BorderTopWidth) solid var(--pf-c-alert--m-inline--BorderColor);
+    border-right: var(--pf-c-alert--m-inline--BorderRightWidth) solid var(--pf-c-alert--m-inline--BorderColor);
+    border-bottom: var(--pf-c-alert--m-inline--BorderBottomWidth) solid var(--pf-c-alert--m-inline--BorderColor);
+    border-left: var(--pf-c-alert--m-inline--BorderLeftWidth) solid var(--pf-c-alert--m-inline--BorderColor);


Since you don't have unique border colors (and I don't think you need them), you could write this shorter like:

border: solid var(--pf-c-alert--m-inline--BorderColor); border-width: var(--pf-c-alert--m-inline--BorderTopWidth) var(--pf-c-alert--m-inline--BorderRightWidth) var(--pf-c-alert--m-inline--BorderBottomWidth) var(--pf-c-alert--m-inline--BorderLeftWidth);

mcoker · 2019-04-30T15:31:06Z

src/patternfly/components/Alert/alert.scss

+    position: absolute;
+    top: var(--pf-c-alert--before--Top);
+    left: 0;
+    z-index: var(--pf-c-alert--before--ZIndex);


Do you need the z-index? Doesn't look like it's necessary. Plus with this high of a z-index, I believe if another component with a lower z-index (like a dropdown menu with --pf-global--ZIndex--xs) pops up over the alert, this border would show up on top of that component.

mcoker · 2019-04-30T15:31:30Z

src/patternfly/components/Alert/alert.scss

@@ -83,14 +91,26 @@
  background-color: var(--pf-c-alert--BackgroundColor);
  box-shadow: var(--pf-c-alert--BoxShadow);

+  // left border treatment base
+  // color is transparent by default, then set by the state modifier classes and turned on by the inline modifier
+  &::before {


What do you think about moving this into the &.pf-m-inline selector since it isn't needed on the default alert?

mcoker · 2019-04-30T15:36:22Z

src/patternfly/components/Alert/alert.scss

+    // left border treatment
+    &::before {
+      background-color: var(--pf-c-alert--m-inline--before--BackgroundColor);
+    }


I don't think you need this since you set --pf-c-alert--before--BackgroundColor: var(--pf-c-alert--m-inline--before--BackgroundColor); above.

mcoker

awesome, thanks! 🥇

mcoker · 2019-05-06T17:22:33Z

src/patternfly/components/Alert/examples/alert-inline-example.hbs

+    {{/alert-title}}
+{{/alert}}
+<br>
+{{#> alert alert--modifier="pf-m-danger pf-m-inline" alert--attribute='aria-label="Inline Danger Alert"'}}


Suggested change

{{#> alert alert--modifier="pf-m-danger pf-m-inline" alert--attribute='aria-label="Inline Danger Alert"'}}

{{#> alert alert--modifier="pf-m-danger pf-m-inline" alert--attribute='aria-label="Inline danger alert"'}}

mcoker · 2019-05-06T17:22:43Z

src/patternfly/components/Alert/examples/alert-inline-example.hbs

+    {{/alert-title}}
+{{/alert}}
+<br>
+{{#> alert alert--modifier="pf-m-warning pf-m-inline" alert--attribute='aria-label="Inline Warning Alert"'}}


Suggested change

{{#> alert alert--modifier="pf-m-warning pf-m-inline" alert--attribute='aria-label="Inline Warning Alert"'}}

{{#> alert alert--modifier="pf-m-warning pf-m-inline" alert--attribute='aria-label="Inline warning alert"'}}

mcoker · 2019-05-06T17:22:53Z

src/patternfly/components/Alert/examples/alert-inline-example.hbs

+    {{/alert-title}}
+{{/alert}}
+<br>
+{{#> alert alert--modifier="pf-m-success pf-m-inline" alert--attribute='aria-label="Inline Success Alert"'}}


Suggested change

{{#> alert alert--modifier="pf-m-success pf-m-inline" alert--attribute='aria-label="Inline Success Alert"'}}

{{#> alert alert--modifier="pf-m-success pf-m-inline" alert--attribute='aria-label="Inline success alert"'}}

mcoker · 2019-05-06T17:23:08Z

src/patternfly/components/Alert/examples/alert-inline-example.hbs

@@ -0,0 +1,35 @@
+{{#> alert alert--modifier="pf-m-info pf-m-inline" alert--attribute='aria-label="Inline Information Alert"'}}


Suggested change

{{#> alert alert--modifier="pf-m-info pf-m-inline" alert--attribute='aria-label="Inline Information Alert"'}}

{{#> alert alert--modifier="pf-m-info pf-m-inline" alert--attribute='aria-label="Inline information alert"'}}

mcoker · 2019-05-06T17:23:34Z

src/patternfly/components/Alert/examples/index.js

@@ -24,6 +27,9 @@ export default () => {
      <Example heading="Alert Variations" handlebars={AlertVariationsRaw}>
        {alertVariations}
      </Example>
+      <Example heading="Inline Alert Types" handlebars={AlertInlineRaw}>


Suggested change

<Example heading="Inline Alert Types" handlebars={AlertInlineRaw}>

<Example heading="Inline alert types" handlebars={AlertInlineRaw}>

mcoker · 2019-05-07T14:32:28Z

src/patternfly/components/Alert/examples/index.js

 import docs from '../docs/code.md';

 export const Docs = docs;

-export default (props) => {
+export default props => {


Can you either rebase or put the () around props?

christiemolloy · 2019-05-13T15:27:26Z

The icon size in the doc vs what you have in the PR, looks different. Could you check with design and see what exact size they want?

christiemolloy · 2019-05-13T15:33:21Z

I think it would also be worth showing an example of an inline notification with a close button

bdellasc · 2019-05-17T16:59:47Z

Good catch, @christiemolloy - it's my bad for not pointing out that icon size tweak to @srambach when we went over the design. I brought the size down when we dropped the BG behind the icon because visually, it feels more like an inline icon next to the title, so having the larger size felt a little too big to me without that BG visually separating it from the text.

mcoker · 2019-05-22T17:18:39Z

src/patternfly/components/Alert/alert.scss

  --pf-c-alert--m-info__title--Color: var(--pf-global--info-color--200);

+  // Inline modification
+  --pf-c-alert--m-inline__title--Color: var(--pf-global--Color--100);


Can you move this var below the icon element vars?

mcoker · 2019-05-22T17:23:05Z

src/patternfly/components/Alert/examples/index.js


 import AlertTypesRaw from '!raw!./alert-types-example.hbs';
 import AlertVariationsRaw from '!raw!./alert-variations-example.hbs';
+import AlertInlineRaw from '!raw!./alert-inline-example.hbs';


can you move this above the AlertVariationsRaw line that comes before it so AlertInline comes before AlertInlineVariations throughout this file?

mcoker · 2019-05-22T17:27:20Z

src/patternfly/components/Alert/alert.scss

+  --pf-c-alert--m-inline--BorderLeftWidth: 0;
+  --pf-c-alert--m-inline--before--Width: var(--pf-global--BorderWidth--lg);
+  --pf-c-alert--m-inline--before--Top: calc(-1 * var(--pf-c-alert--m-inline--BorderTopWidth));
+  --pf-c-alert--m-inline--before--Height: calc(100% + var(--pf-c-alert--m-inline--BorderTopWidth) + var(--pf-c-alert--m-inline--BorderBottomWidth));


this works, but you could also remove height and use bottom instead as --pf-c-alert--m-inline--before--Bottom: calc(-1 * var(--pf-c-alert--m-inline--BorderBottomWidth)); (the same way you define top)

mcoker

lgtm! just a few small things I commented on.

mcoker

🥇

matthewcarleton

Nice Work!!!

patternfly-build · 2019-05-23T17:00:20Z

🎉 This PR is included in version 2.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

* feat(alert): adds inline modifier

srambach requested review from mceledonia and mcoker April 29, 2019 17:45

mcoker reviewed Apr 29, 2019

View reviewed changes

mcoker reviewed Apr 30, 2019

View reviewed changes

mcoker approved these changes Apr 30, 2019

View reviewed changes

mcoker reviewed May 6, 2019

View reviewed changes

srambach force-pushed the inline-alert-variation-1390 branch 2 times, most recently from 88468a3 to 8cdf421 Compare May 6, 2019 18:22

mcoker reviewed May 7, 2019

View reviewed changes

matthewcarleton mentioned this pull request May 13, 2019

Adding clone-vm design to knikubevirt folder openshift/openshift-origin-design#168

Merged

srambach added 5 commits May 21, 2019 15:47

feat(alert): adds inline modifier

6da8602

feat(alert): moves left border to pseudoelement

fa70200

feat(alert): simplify pseudoelement border

6fa112f

feat(alert): correct sentence case

50577bd

feat(alert): adds variations and adjusts icon

633b296

feat(alert): rebase

4498f53

srambach force-pushed the inline-alert-variation-1390 branch from 6ccf111 to 4498f53 Compare May 21, 2019 19:57

srambach requested review from bdellasc, matthewcarleton and mcoker May 22, 2019 12:37

mcoker reviewed May 22, 2019

View reviewed changes

srambach added 3 commits May 22, 2019 13:40

feat(alert): rearrange and add raw import

b02ccc3

feat(alert): remove comment

207c345

feat(alert): fix variable and comment

a6651af

mcoker approved these changes May 22, 2019

View reviewed changes

matthewcarleton approved these changes May 23, 2019

View reviewed changes

matthewcarleton merged commit 04641c9 into patternfly:master May 23, 2019

patternfly-build added the released label May 23, 2019

srambach referenced this pull request in mcoker/patternfly-next May 28, 2019

feat(alert): adds inline modifier (#1775)

3988858

* feat(alert): adds inline modifier

	{{#> alert alert--modifier="pf-m-danger pf-m-inline" alert--attribute='aria-label="Inline Danger Alert"'}}
	{{#> alert alert--modifier="pf-m-danger pf-m-inline" alert--attribute='aria-label="Inline danger alert"'}}

	{{#> alert alert--modifier="pf-m-warning pf-m-inline" alert--attribute='aria-label="Inline Warning Alert"'}}
	{{#> alert alert--modifier="pf-m-warning pf-m-inline" alert--attribute='aria-label="Inline warning alert"'}}

	{{#> alert alert--modifier="pf-m-success pf-m-inline" alert--attribute='aria-label="Inline Success Alert"'}}
	{{#> alert alert--modifier="pf-m-success pf-m-inline" alert--attribute='aria-label="Inline success alert"'}}

		@@ -0,0 +1,35 @@
		{{#> alert alert--modifier="pf-m-info pf-m-inline" alert--attribute='aria-label="Inline Information Alert"'}}

	<Example heading="Inline Alert Types" handlebars={AlertInlineRaw}>
	<Example heading="Inline alert types" handlebars={AlertInlineRaw}>

Conversation

srambach commented Apr 29, 2019

Uh oh!

patternfly-build commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdellasc commented Apr 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

bdellasc commented Apr 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christiemolloy commented May 13, 2019

Uh oh!

christiemolloy commented May 13, 2019

Uh oh!

bdellasc commented May 17, 2019

Uh oh!

mcoker May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

matthewcarleton left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented May 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

patternfly-build commented Apr 29, 2019 •

edited

Loading

mcoker May 22, 2019 •

edited

Loading