Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding an internal theme #253

Merged
merged 12 commits into from
Jul 19, 2022
Merged

Conversation

stephiescastle
Copy link
Member

@stephiescastle stephiescastle commented Jun 13, 2022

Checklist

  • Include a description of your pull request and instructions for the reviewer to verify your work.
  • Link to the issue if this PR is issue-specific.
  • Create/update the corresponding story if this includes a UI component.
  • Create/update documentation. If not included, tell us why.
  • List the environments / browsers in which you tested your changes.
  • Tests, linting, or other required checks are passing.
  • PR has an informative and human-readable title
    • PR titles are used to generate the change log in releases; good ones make that easier to scan.
    • PRs will be broadly categorized in the change log, but for even easier scanning, consider prefixing with a component name or other useful categorization, e.g., "BaseButton: fix layout bug", or "Storybook: Update dependencies".
  • PR has been tagged with a SemVer label and a general category label, or skip-changelog.
    • These tags are used to do the aforementioned broad categorization in the change log and determine what the next release's version number should be.
    • Release Drafter will attempt to do the category labeling for you! Please double-check its work.

Description

This is a start to "combining the storybooks." It really only covers adding the internal styles. Some implementation notes:

  • I went with the approach discussed in #9. Users who want to use the internal theme will apply a body class of ThemeInternal to their site.
  • Unfortunately, we did not have a .BaseLink class in place on our BaseLink component template, so users that want to start using the internal theme will need to update a lot of their component templates. On the other hand, we may not have that many users, so perhaps not a big worry.
  • Currently, the internal theme styles are separated by component (added to existing component scss files). I went back and forth between this and the alternate approach of having all internal styles in a dedicated SCSS file. At the time, it seemed more sensible to have component SCSS grouped together for comparing rules, but it does have the unfortunate effect of sometimes needing to add a SCSS file for a component just for the internal theme (ex: _BlockAccordion.scss). It will also make it more difficult to create a dedicated internal.min.css file as discussed. I think I'm going to change the approach and move all internal theme styles to a single SCSS file, but I'm definitely interested in other devs thoughts on this before I make that change.
  • The documentation for how to use the internal theme is pretty simple. See the Foundation: Themes page in the storybook.
  • When we add internal and external components, I'm not planning on creating separate categories. I prefer @Scotchester's reccommendation to specify HeaderInternal and HeaderExternal. Then in storybook we can structure the documentation like so:
Components
└── Header
    ├── External
    └── Internal

Instructions to test

  1. npm run storybook
  2. View a component, or even a component overview page. Use the theme switcher to use the internal theme. Note that any stories using an iframe to display a component on a docs page will ignore the theme switcher (usually anything that requires additional javascript). I'd really like to fix that issue in storybook, as it also leads to this problem. Hoping to get that prioritized soon.
  3. See the documentation on how to implement the internal theme on your project on the Foundation: Themes page.

Tested in the following environments/browsers:

Operating System

  • macOS
  • iOS
  • iPadOS
  • Windows

Browser

  • Chrome
  • Firefox ESR
  • Firefox
  • Safari
  • Edge

@stephiescastle stephiescastle added storybook This issue relates to Storybook.js minor Contains new features or enhancements feature labels Jun 13, 2022
@stephiescastle
Copy link
Member Author

stephiescastle commented Jun 21, 2022

Update: I'm reworking this PR to:

  • group all internal styles in a single scss file
  • use @apply in BaseLink and BlockAccordion for both the default and internal theme colors
  • rework the docs per code review

Copy link
Member Author

@stephiescastle stephiescastle left a comment

Choose a reason for hiding this comment

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

@Scotchester I've updated this PR to address the following:

  • group all internal styles in a single scss file
  • use @apply in BaseLink and BlockAccordion for both the default and internal theme colors not needed anymore, see write-up below
  • rework the docs per code review

In doing this work, I discovered I also needed to rework the .text-theme-red classes. I decided to expand on it by adding .ThemeInternal styles for it as well. The name also no longer made sense, so I renamed those classes to .text-theme-color and .text-theme-color-hover. I only deprecated the old class names, so this isn't a breaking change. This also greatly simplifies the scss overrides needed in most of our components. We only need them for BaseButton now. It also negates the need for having a .BaseLink class applied to every link. The documentation has also been updated regarding the "Adaptive Text Colors."

I also updated BlockRelatedLinks to have an internal theme (an oversight from the previous work). Thanks to the updated text classes, this was just a matter of changing the inline class from text-jpl-red to text-theme-color.

Migration notes

Internal themes have been added to Explorer 1. To use them, add .ThemeInternal as a body class to your project. If you are compiling your own scss assets, be sure to import the new /src/scss/themes/_internal.scss file.

.text-theme-red and .text-theme-red-hover are now deprecated and have been superseded by .text-theme-color and .text-theme-color-hover. While the old class names will still work, they will be removed in the next major release. To prevent any unexpected style changes, it's recommended to update your class names now.

Update the templates of the following components to use the new .text-theme-color and .text-theme-color-hover classes:

  • BaseLink
  • BlockRelatedLink
  • BlockAccordionItem
  • MixinAnimationCaret

src/scss/_typography_theme_colors.scss Outdated Show resolved Hide resolved
Copy link
Member

@Scotchester Scotchester left a comment

Choose a reason for hiding this comment

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

Looking very good! Just a few final questions.

src/scss/_typography_theme_colors.scss Outdated Show resolved Hide resolved
src/scss/_typography_theme_colors.scss Outdated Show resolved Hide resolved
src/scss/themes/_internal.scss Show resolved Hide resolved
@stephiescastle
Copy link
Member Author

Thanks for the review @Scotchester! I've updated the filenames and responded to your questions.

@stephiescastle stephiescastle linked an issue Jul 19, 2022 that may be closed by this pull request
10 tasks
@stephiescastle stephiescastle merged commit bb44f14 into main Jul 19, 2022
@stephiescastle stephiescastle deleted the feature/9-adding-internal-theme branch July 19, 2022 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature minor Contains new features or enhancements storybook This issue relates to Storybook.js
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combine Storybooks
2 participants