[draft] fix: tied legacy pfe-cta css vars to the new color-context vars

[heyMP]

Problem / Solution

This illustrates how to tie the legacy CSS vars for pfe-cta into the new color context controller --pfe-context-background-color: var(--pfe-cta--BackgroundColor);

The _sharedStyles.scss generates all of the color-palette and priority overrides via css custom properties. Since the color context provider sets background-color and only background-color, we have to specifically use that custom property to override background-color.

color-context.scss

:host(:is([color-palette])) {
  background-color: var(--pfe-context-background-color, var(--pfe-theme--color--surface--base));
}

Alternatives

• fix: cta styling removing color context provider #2005
• We could also remove the background-color declaration in color-context.scss and put in the docs that you also have to implement the --pfe-context-background-color manually.

Related issues

• pfe-cta color context attribute color-palette incorrectly applies styles to variants #2002

Preview

Link(s) to demo page(s) where this element can be viewed:

• Link

What has changed and why

Testing instructions

• Test case upgraded storybook to the latest alpha release and added a command fo… #1
  1. Go here
  2. Do this
• Test case adding command for new RHElement using generator-rhelement #2
  1. Go here
  2. Do this

Browser requirements

Your component should work in all of the following environments:

• Latest 2 versions of Edge
• Internet Explorer 11 (should be useable, not pixel perfect)
• Latest 2 versions of Firefox (one on Mac OS, one of Windows OS)
• Firefox 78 (or latest version for Red Hat Enterprise Linux distribution)
• Latest 2 versions of Chrome (one on Mac OS, one of Windows OS)
• Latest 2 versions of Safari
• Android mobile device (such as the Galaxy S9)
• Apple mobile device (such as the iPhone X)
• Apple tablet device (such as the iPhone Pro)

Ready-for-merge Checklist

• Expected files: all files in this pull request are related to one request or issue (no stragglers or scope-creep).
• Tests have been updated to cover these changes.
• Browser testing passed.
• Changelog updated.
• Documentation (README.md, WHY.md, etc.) updated or added.
• Link to the demo recording:
• Approved by designer.

Merging

Please squash when merging and ensure your commit message uses conventional commit formatting.

Be sure to share your updates with the [email protected] mailing list!

[changeset-bot]

🦋 Changeset detected

Latest commit: 814e5f8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@patternfly/pfe-cta	Patch
@patternfly/pfe-tools	Minor
@patternfly/pfe-core	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[netlify]

✅ Deploy Preview for patternfly-elements ready!

Name	Link
🔨 Latest commit	814e5f8
🔍 Latest deploy log	https://app.netlify.com/sites/patternfly-elements/deploys/626a4dae768828000976b16a
😎 Deploy Preview	https://deploy-preview-2006--patternfly-elements.netlify.app/blog/pfe-2-0-prerelease
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[bennypowers]

I pushed some commits which fix up the context variables for CTA by explicitly defining them. Is is the prettiest code? No. However I'd prefer not to dwell on it, since I'm fixing to refactor this CSS at some point anyway. It would be nice to add some regression tests on this branch once we reach consensus (through manual testing) that it passes acceptance criteria.

I haven't yet addressed the custom --context div wrapper, will tackle that next.

I tested this by running two separate dev servers, one with commit c10f678 checked out (i.e. before the context refactor) and one with this branch checked out.

In these screenshots, the left hand side is the old, pre-refactor version, and the right hand side is the PR branch

[bennypowers]

OK the issue with the --context div is that the refactor moved responsibility for deriving the context from the css var to the provider only. This was a performance enhancement as much as it was about simplifying the code. So let's reopen that discussion: Should we stop supporting --context without a provider element?

Opposed:

• That means changing the pfe-cta demo file, which is another way of saying making a breaking change to our acceptance criteria
• There may be cases of such 'in the wild' which will break (@zhawkins reports that redhat.com does not appear to use --context as such)

In Favour:

• --context is not documented on patternflyelements.org
• Context is JS-only anyways, so we don't lose by doubling down on custom element registration
• Allows us to simplify context code and improve performance

We could take a compromise position and require on=${intendedContext} in markup.

You'll notice that one of the examples in the demo came with it's own on attr in markup, so perhaps that was the intention?

patternfly-elements/elements/pfe-cta/demo/pfe-cta.html

Line 192 in c10f678

<pfe-cta on="saturated"><a href="https://www.redhat.com#default">Default</a></pfe-cta>

A fourth option would be to add escape hatches to ContextConsumer for specific cases

Practically speaking...

So, the practical options before us as I see them ATM are:

1. partially revert context rewrite to support orphaned --context var
2. decide to stop supporting same and rewrite the demos (per @heyMP)
3. compromise and require on in markup
4. add escape hatches to ContextConsumer for cases like this

[zeroedin]

I think the compromise with on might fit the bill here. While it isn't ideal to have to set the attribute individually when used inside a parent that isn't a color context provider, it provides flexibility and performance by not having to need additional js.

[bennypowers]

Ok, taking @heyMP and @zeroedin 's comments into consideration, what I will do is:

• explicitly add on to demo examples
• add theming examples to demo pages which place CTAs on context providers, but customize documented theme vars
• add regression tests for CTA

There are a few other examples in the repo where --context is set, so i'll examine those, and if I find the same issue there i'll take the same approach

[bennypowers]

@heyMP @zeroedin

• --context by itself is no longer supported (it was never documented). Authors that want to 'fake' a context should provide both --context and on to each element. And actually, they shouldn't ever do that.
• on set in HTML now overrides color context for providers. in other words, manually setting on is an escape hatch which opts out of color context calculations. Currently there's no way to later on imperatively opt back in.
  • docs do mention the on attribute, but imply that it is 'behind the scenes'. perhaps we should make this explicit in docs?
  • elsewhere in docs pfe-theme is documented as an override for on, this should be updated, right?
• adds some test helpers for validating css color strings

compare this PR (https://deploy-preview-2006--patternfly-elements.netlify.app/components/cta/demo/) with an old version (https://deploy-preview-1989--patternfly-elements.netlify.app/components/cta/demo/)

[bennypowers]

Less griping, tinker more!