feat(banner): add link style #4383

srambach · 2021-09-20T20:46:20Z

This adds styles for inline links in the banner.
@mceledonia @lboehling I don't know if we want to show all these examples or not, but I put them there for you to take a look at. Thanks!

Fixes #4360

patternfly-build · 2021-09-20T21:01:23Z

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

lboehling · 2021-09-21T15:41:46Z

looks great!!

mcoker · 2021-09-21T22:34:11Z

src/patternfly/components/Banner/banner.scss

@@ -57,4 +57,18 @@
    z-index: var(--pf-c-banner--m-sticky--ZIndex);
    box-shadow: var(--pf-c-banner--m-sticky--BoxShadow);
  }
+
+  .pf-c-button.pf-m-link.pf-m-inline {


Looks like the styles added are limited to the inline link button component - is that intended @mcarrano or should a user be able to add a regular link, too?

I was assuming a regular link.

I added that. @mcarrano which examples do you want to show? Do we need all these? I put them there to make sure everything works but I'm not sure if we need to show them all as examples or not.

@srambach Yes, I do feel like we have too many examples here. What's the difference between a link and an inline link? I think that the Call to action example should be removed as it's not something we recommend that banners be used for. I'm on the fence about whether we need to allow for both inline text links and link buttons. The banner was originally designed to hold short text messages like an alert. It's reasonable that you might include a text link in that message. A button implies to me some action that probably does not belong on a banner. Put I'm OK to leave it there as there might be a use case I'm not considering.

@mceledonia @lboehling I also want to get your input/feedback about the styling. Looks like the current approach is to make the link text bold and then underline on hover. I'm not sure that bold text affords a link to me. Would just underlining the link be more clear (in it's default state)?

Also, although I was able to make the not-allowed cursor on the disabled button link, there are good a11y reasons not to try to make a link look disabled. Also I had to turn pointer events back on to do the cursor change for the link button, which isn't really good to do. So maybe we forget about that part?
And if we don't want to support button links, I can either take that out or leave it in just in case, and remove the examples.
I'll wait to hear about about the bold/underline.

mcoker · 2021-09-21T22:38:18Z

src/patternfly/components/Banner/banner.scss

+
+  .pf-c-button.pf-m-link.pf-m-inline {
+    --pf-c-button--FontWeight: var(--pf-global--FontWeight--semi-bold);
+    --pf-c-button--m-link--Color: var(--pf-c-banner--Color);


I would create vars for these values instead of reusing the banner text color and using a global var for the font-weight. Then they're customizable by the user via a var without having to write out the .pf-c-button.pf-m-link.pf-m-inline selector and redefining the button vars.

Maybe something like

// top of scss --pf-c-banner--link--FontWeight: var(--pf-global--FontWeight--semi-bold); --pf-c-banner--link--Color: var(--pf-c-banner--Color); --pf-c-banner--link--hover--Color: var(--pf-c-banner--Color); --pf-c-banner--link--disabled--Color: var(--pf-c-banner--Color); // here --pf-c-button--FontWeight: var(--pf-c-banner--link--FontWeight); --pf-c-button--m-link--Color: var(--pf-c-banner--link--Color); ...

I can remove .pf-m-inline, but .pf-c-button.pf-m-link from the button component will override the variables if I don't create more specificity here.

mcoker

LGTM! Just a couple of small things.

mcoker · 2021-09-23T17:22:20Z

src/patternfly/components/Banner/banner.scss

+    color: var(--pf-c-banner--link--Color);
+
+    &:hover {
+      color: var(--pf-c-banner--link--hover--Color);


Suggested change

color: var(--pf-c-banner--link--hover--Color);

--pf-c-banner--link--Color: var(--pf-c-banner--link--hover--Color);

mcoker · 2021-09-23T17:22:29Z

src/patternfly/components/Banner/banner.scss

+
+    // stylelint-disable selector-no-qualifying-type
+    &.pf-m-disabled {
+      color: var(--pf-c-banner--link--disabled--Color);


Suggested change

color: var(--pf-c-banner--link--disabled--Color);

--pf-c-banner--link--Color: var(--pf-c-banner--link--disabled--Color);

mcoker · 2021-09-23T17:26:40Z

src/patternfly/components/Banner/banner.scss

+    // stylelint-enable selector-no-qualifying-type
+  }
+
+  .pf-c-button.pf-m-link {


targeting .pf-m-inline is probably more ideal here since that's what we want to style. I suspect we want to leave the plain link button variant alone, as we would the other button variants like primary, secondary, tertiary, etc. my previous comment was just about being able to target the inline button links using link vars instead of always being tied to the banner color, which you've done here.

mcoker · 2021-09-23T17:27:45Z

src/patternfly/components/Banner/examples/Banner.md

+  {{/button}}
+{{/banner}}
+<br>
+{{#> banner banner--modifier="pf-m-info"}}


looks like @mcarrano wants to remove this example

@mcoker @mcarrano Do we actually need most of these examples? I wanted to make sure they all worked, but should we simply add the one preferred method (which one?) of using a link within the banner into one or all of the existing examples?

@srambach @mcoker I would be in favor of that. Maybe just update the existing examples to include a link in the message text. That way you would see what the link looks like for each color background.

mcoker · 2021-09-29T17:51:07Z

src/patternfly/components/Banner/banner.scss

+    // stylelint-enable selector-no-qualifying-type
+  }
+
+  .pf-m-inline {


This needs to be scoped so it doesn't apply styles to some other component with .pf-m-inline

mcoker · 2021-09-29T17:51:46Z

@mcarrano ready for re-review

mcarrano

Looks good to me. Thanks for making these changes @srambach .

mcoker

LGTM!!!

patternfly-build · 2021-09-29T18:59:32Z

🎉 This PR is included in version 4.140.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat(banner): add link style

6a4a38f

mcoker requested changes Sep 21, 2021

View reviewed changes

feat(banner): add variable, link styling

f185d64

mcoker requested changes Sep 23, 2021

View reviewed changes

mcoker requested review from lboehling and mceledonia September 28, 2021 20:35

srambach added 2 commits September 29, 2021 09:05

feat(banner): address comments

109aa32

feat(banner): restyle, consolidate examples

9032499

mcoker requested changes Sep 29, 2021

View reviewed changes

mcarrano approved these changes Sep 29, 2021

View reviewed changes

feat(banner): scope to inline button

7333895

mcoker approved these changes Sep 29, 2021

View reviewed changes

mcoker merged commit 1515097 into patternfly:main Sep 29, 2021

patternfly-build added the released label Sep 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(banner): add link style #4383

feat(banner): add link style #4383

srambach commented Sep 20, 2021

patternfly-build commented Sep 20, 2021 •

edited

Loading

lboehling commented Sep 21, 2021

mcoker Sep 21, 2021

mcarrano Sep 22, 2021

srambach Sep 22, 2021

mcarrano Sep 22, 2021

srambach Sep 22, 2021

mcoker Sep 21, 2021

srambach Sep 22, 2021

mcoker left a comment

mcoker Sep 23, 2021

mcoker Sep 23, 2021

mcoker Sep 23, 2021

mcoker Sep 23, 2021

srambach Sep 29, 2021

mcarrano Sep 29, 2021

mcoker Sep 29, 2021

mcoker commented Sep 29, 2021

mcarrano left a comment

mcoker left a comment

patternfly-build commented Sep 29, 2021

	color: var(--pf-c-banner--link--hover--Color);
	--pf-c-banner--link--Color: var(--pf-c-banner--link--hover--Color);

	color: var(--pf-c-banner--link--disabled--Color);
	--pf-c-banner--link--Color: var(--pf-c-banner--link--disabled--Color);

feat(banner): add link style #4383

feat(banner): add link style #4383

Conversation

srambach commented Sep 20, 2021

patternfly-build commented Sep 20, 2021 • edited Loading

lboehling commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoker commented Sep 29, 2021

mcarrano left a comment

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

patternfly-build commented Sep 29, 2021

patternfly-build commented Sep 20, 2021 •

edited

Loading