Skip to content

DE21128-broadcast-theme-hooks #642

Merged
starryeyez024 merged 7 commits intomasterfrom
DE21128-broadcast-theme-hooks
Dec 2, 2019
Merged

DE21128-broadcast-theme-hooks #642
starryeyez024 merged 7 commits intomasterfrom
DE21128-broadcast-theme-hooks

Conversation

@starryeyez024
Copy link
Copy Markdown
Member

@starryeyez024 starryeyez024 commented Nov 26, 2019

What has changed and why

  • Added support for manual overrides to which theme context is invoked by surface colors. Previously the theme was "locked" into having 3 light theme colors, 2 dark, and 2 saturated. Now the themes can be redefined if desired.
  • Added demo file for pfe-card to test.

Testing instructions

Be sure to include detailed instructions on how your update can be tested by another developer.

  1. Open the elements/pfe-card/demo/custom-theme.css
  2. Change the values of the following variables. Make stuff up, but make the colors match the appropriate theme, like this:
  --pfe-theme--color--surface--lightest: purple;
  --pfe-theme--color--surface--lightest--theme: dark;
  --pfe-theme--color--surface--lighter: maroon;
  --pfe-theme--color--surface--lighter--theme: dark;
  --pfe-theme--color--surface--base: green;
  --pfe-theme--color--surface--base--theme: dark;
  --pfe-theme--color--surface--darker: pink;
  --pfe-theme--color--surface--darker--theme: light;
  --pfe-theme--color--surface--darkest: yellow;
  --pfe-theme--color--surface--darkest--theme: light;
  --pfe-theme--color--surface--complement: hotpink;
  --pfe-theme--color--surface--complement--theme: saturated;
  --pfe-theme--color--surface--accent: #c0ff33;
  --pfe-theme--color--surface--accent--theme: light;
  1. The cards should reflect both the color and theme changes.

image

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 60.7.2 (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
  • Galaxy S9 Firefox
  • iPhone X Safari
  • iPad Pro Safari
  • Pixel 3 Chrome

Ready-for-merge Checklist

  • Expected files: all files in this pull request are related to one feature request or issue (no stragglers)?
  • Did browser testing pass?
  • Did you update or add any necessary documentation (README.md, WHY.md, etc.)?
  • Was this feature demo'd and the design review approved?
  • Did you update the CHANGELOG.md file with a summary of this update?

…ch theme context is invoked by surface colors. Add demo file for this.
Comment thread elements/pfe-sass/mixins/_custom-properties.scss Outdated
Comment thread elements/pfe-card/demo/custom-theme.css Outdated
Comment thread elements/pfe-card/demo/custom-theme.css Outdated
Comment thread elements/pfe-card/demo/custom-theme.css Outdated
Comment thread elements/pfe-sass/mixins/_custom-properties.scss Outdated
@castastrophe castastrophe added ready: branch testing Test the component from a user-perspective. Try to break it! ready: browser testing Test the component in the supported browser environments. needs changelog Be sure to update the Changelog before merging. needs code updates Code updates have been requested. styles An issue or PR pertaining only to CSS/Sass labels Dec 2, 2019
LyndseyR
LyndseyR previously approved these changes Dec 2, 2019
Comment thread elements/pfe-card/demo/custom-theme.css Outdated
Comment thread elements/pfe-card/demo/custom-theme.css Outdated
castastrophe
castastrophe previously approved these changes Dec 2, 2019
Copy link
Copy Markdown
Contributor

@castastrophe castastrophe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just needs a changelog

…/patternfly-elements into DE21128-broadcast-theme-hooks
@starryeyez024 starryeyez024 dismissed stale reviews from castastrophe and LyndseyR via abfd9bb December 2, 2019 18:05
@starryeyez024
Copy link
Copy Markdown
Member Author

@castastrophe added!

Copy link
Copy Markdown
Contributor

@kylebuch8 kylebuch8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lady Gaga... Thy Manners!!!

@starryeyez024 starryeyez024 merged commit 5480888 into master Dec 2, 2019
@castastrophe castastrophe deleted the DE21128-broadcast-theme-hooks branch December 2, 2019 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs changelog Be sure to update the Changelog before merging. needs code updates Code updates have been requested. ready: branch testing Test the component from a user-perspective. Try to break it! ready: browser testing Test the component in the supported browser environments. 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