Skip to content

Conversation

@zeroedin
Copy link
Contributor

Problem

pfe-cta had color-palette attribute and the @colorContextProvider() decorator applied to it inadvertently causing the variants to display colors incorrectly when inheriting from their parent color context and applying their own overrides.

Related issues

Solution

Remove the @colorContextProvider() decorator and revert back to color to a standard property. Also removing the @deprecated decorator as it is in this case not a part of the color context controller. Update the demo HTML and component scss to reflect this reversion.

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 patternfly-elements-contribute@redhat.com mailing list!

@changeset-bot
Copy link

changeset-bot bot commented Apr 18, 2022

🦋 Changeset detected

Latest commit: e8bef3b

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

This PR includes changesets to release 1 package
Name Type
@patternfly/pfe-cta 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

@github-actions github-actions bot added demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass labels Apr 18, 2022
@zeroedin zeroedin requested review from bennypowers and heyMP April 18, 2022 19:13
@github-actions github-actions bot added the AT passed Automated testing has passed label Apr 18, 2022
@zeroedin
Copy link
Contributor Author

Something to note, these changes do not address the usage of the custom --context CSS property use case in the demo. We should most likely take a look at that as a part of this PR as well.

image

vs. 1.x

image

@bennypowers
Copy link
Member

bennypowers commented Apr 19, 2022

If color took values like red or yellow it would make sense to me to keep it, like label or badge. But since it takes ColorTheme values like base or accent, imo it'd be better to just fix these specific cases. We could document that it only takes a subset of those ColorPalette values, and if any code is needed, add an option to the colorContextProvider decorator. It might just need fixes in lightdom styles, or in ::slotted.

P.S. apologies for the terseness while I'm PTO. happy to talk it over sync next week if needed.

@zeroedin
Copy link
Contributor Author

P.S. apologies for the terseness while I'm PTO. happy to talk it over sync next week if needed.

No worries, we didn't think this was a blocker of anything, we just had a conversation about it yesterday so I wanted to get something down for people to review and bring up these comments. Enjoy your PTO, no more work. :)

@heyMP
Copy link
Contributor

heyMP commented Apr 19, 2022

@zeroedin Give this a glance and see what you think. This would keep the color context provider within pfe-cta. I think it still needs some more css rules but it fixes most of the issues of color context background color overriding pfe-cta styles. #2006

@bennypowers
Copy link
Member

superceded by #2006

@zeroedin zeroedin deleted the fix/cta-styling-color-context branch August 25, 2023 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AT passed Automated testing has passed demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants