feat(subnav): add support for dark theme #381

zchsh · 2021-10-04T22:21:29Z

🎟️ Asana Task
🔍 Preview Link
📖 Storybook Preview
🐣 Canary Test Preview via https://github.com/hashicorp/cloud.hashicorp.com/pull/192

This PR adds support for a theme prop to @hashicorp/react-subnav. The intent here is to enable a dark theme, as implemented in cloud.hashicorp.com (see preview) without using potentially brittle overrides (which are also now much harder to write at the project level due to our use of CSS modules).

PR Checklist 🚀

Items in this checklist may not may not apply to your PR, but please consider each item carefully.

Add Asana and Preview links above.
Conduct thorough self-review.
Add or update tests as appropriate.
Conduct reasonable cross browser testing for both compatibility and responsive behavior (We have a Sauce Labs account for this, if you don't have access, just ask!).
Conduct reasonable accessibility review (use the WAS as a guide or an axe browser plugin until we establish more formal checks).
Identify (in the description above) and document (add Asana tasks on this board) any technical debt that you're aware of, but are not addressing as part of this PR.

changeset-bot · 2021-10-04T22:21:32Z

🦋 Changeset detected

Latest commit: ae7e4a1

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

This PR includes changesets to release 1 package

Name	Type
@hashicorp/react-subnav	Minor

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

vercel · 2021-10-04T22:21:32Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/hashicorp/react-components/2o4EoXBxQBsCj7EFRpsUnGpBUQWX
✅ Preview: https://react-components-git-zssubnav-dark-theme-hashicorp.vercel.app

BRKalow · 2021-10-04T22:32:36Z

packages/subnav/theme.module.css

@@ -0,0 +1,43 @@
+/* Theme settings. Note: also consumed in /partials */


zchsh · 2021-10-05T01:13:20Z

packages/subnav/index.js

@@ -2,6 +2,7 @@ import classNames from 'classnames'
 import useProductMeta from '@hashicorp/platform-product-meta'
 import useNavRef from './helpers/useNavRef'
 import s from './style.module.css'
+import themeStyles from './theme.module.css'


Took a theming approach similar to that in code-block. While I am certainly not confident this will be the best approach to theming long-term (in fact I'm almost sure we can come up with something more extensible for product situations), this felt like a decent balance between using our existing tooling and conventions (CSS modules & custom properties) while splitting out theme properties.

My hot take is I don't think the theming is something we should aim to "solve" with this PR, really just trying to get this to work and be easy to transition to whatever theming approach we go with longer term. Very open to suggestions here, of course 🙇

zchsh · 2021-10-05T01:13:54Z

packages/subnav/index.js

@@ -115,7 +119,7 @@ function Subnav({ className, ...restProps }) {
  return (
    <nav
      ref={wrapperRef}
-      className={classNames(s.root, className, {
+      className={classNames(s.root, themeStyles[theme], className, {


MOST of the theming is done via CSS custom properties - that comes from this class here 👍

zchsh · 2021-10-05T01:14:45Z

packages/subnav/index.js

@@ -125,6 +129,7 @@ function Subnav({ className, ...restProps }) {
        product={product}
        hasOverflow={hasOverflow}
        isSticky={isSticky}
+        theme={theme}


SOME of the theming, namely just the theme passed to button's theme.background, has to be passed through the CtaLinks component. This is a part that doesn't feel ideal long term (Context probably better?) but felt sustainable and reasonable given the scope & intent of this PR.

zchsh · 2021-10-05T01:15:58Z

packages/subnav/partials/CtaLinks/github-stars-link/style.module.css

-  --stars-border-color: rgba(29, 30, 35, 0.2);
-  --scrim-opacity: 0;
+  --local-scrim-color: transparent;
+  --local-divider-color: var(--border-color); /* from theme.module.css */


These --local vars make it possible to group styles fully by element. Without them I think we'd have to group by state, which might be okay in this specific situation, but quickly gets messy so I think should be avoided (per CSS guidelines in Notion - though as always, open to discussion on those as well).

zchsh · 2021-10-05T01:16:23Z

packages/subnav/partials/CtaLinks/icons/github-logo.js

@@ -5,6 +5,7 @@ function GithubIcon(props) {
    <svg width={16} height={16} viewBox="0 0 16 16" {...props}>
      <path
        fillRule="evenodd"
+        fill="#000000"


Added this as it's usually present in icon exports, and makes it possible to target the icon's fill in a less brittle way.

zchsh · 2021-10-05T01:17:21Z

packages/subnav/partials/CtaLinks/index.js

@@ -41,6 +40,8 @@ function CtaLinks(props) {
            theme={{
              brand: product,
              variant: isLastButton ? 'primary' : 'secondary',
+              background: theme,
+              ...link.theme /* allow theme overrides via the ctaLinks array */,


Added this in to remove a related override in cloud.hashicorp.com used to emulate a secondary button. Consumers can now add a link.theme attribute to set button themes, if desired.

zchsh · 2021-10-05T01:24:35Z

packages/subnav/theme.module.css

+  --menu-item-heading-text-color: var(--gray-3);
+}
+
+.dark {


I've riffed off the existing implementation at cloud.hashicorp.com. Have also changed the modals to match the dark theme of the bar to avoid a checkerboard look.

Not 100% sure if this matches design's intent - I found what I think is the reference Figma file but could not find deets on the nav theming. So, went with what I think would match design intent.

github-actions · 2021-10-05T01:27:13Z

📦 Canary Packages Published

Latest commit: ae7e4a1

Published 1 packages

@hashicorp/react-subnav@9.1.0-canary-20219522528

npm install @hashicorp/react-subnav@canary

BRKalow · 2021-10-05T02:09:33Z

Nice work here! I think the only thing that I see in the HCP preview is that the logo svg paths have a black fill. 💭

zchsh · 2021-10-05T02:14:01Z

@BRKalow ugh, aack... yeah the CSS there is pretty brittle. Will switch to loading in the white logos proper 👍 Extra weight but probably worth the stability.

zchsh · 2021-10-05T02:32:54Z

Had some late-night-still-pretty-sick brain there but https://cloud-hashicorp-com-git-zsupgrade-subnav-hashicorp.vercel.app should be fixed now with all the new logos 👍

Note: also tested out all the light / dark and product combos on the Storybook preview to make sure they're all loading.

BRKalow

This all looks good to me, thanks for tackling this! Nice balance of get-it-out-the-door and forward-thinking 🙇

zchsh added 4 commits October 4, 2021 18:20

feat(subnav): WIP implement dark theme

d095c2d

feat(subnav): finish implementing dark theme

591713d

chore(subnav): clean up old comment

ac83bda

fix(subnav): revert changes to useStuckRef

a455691

BRKalow reviewed Oct 4, 2021

View reviewed changes

packages/subnav/theme.module.css Outdated

@@ -0,0 +1,43 @@

/* Theme settings. Note: also consumed in /partials */

Copy link

Contributor

BRKalow Oct 4, 2021

Choose a reason for hiding this comment

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

❤️

chore(subnav): add changeset

fffb541

vercel bot deployed to Preview October 5, 2021 00:50 View deployment

fix(subnav): fix issue with logo sizing

abee116

vercel bot deployed to Preview October 5, 2021 01:05 View deployment

docs(subnav): document new theme prop for ctaLinks

e6367e1

vercel bot deployed to Preview October 5, 2021 01:18 View deployment

chore(subnav): clarify comments on theme.module.css

0371e0e

vercel bot deployed to Preview October 5, 2021 01:22 View deployment

zchsh commented Oct 5, 2021

View reviewed changes

zchsh changed the title ~~[WIP] feat(subnav): add support for dark theme~~ feat(subnav): add support for dark theme Oct 5, 2021

zchsh added the release:canary Triggers a canary release for commits to this pull request label Oct 5, 2021

zchsh requested a review from BRKalow October 5, 2021 02:08

fix: issue with CSS targeting logo color

69ac332

fix: use proper colorwhite logos for dark theme

36a62a5

vercel bot had a problem deploying to Preview October 5, 2021 02:18 Failure

fix: typo in subnav CSS

c34638a

vercel bot deployed to Preview October 5, 2021 02:21 View deployment

zchsh marked this pull request as ready for review October 5, 2021 02:22

fix: missing theme prop for TitleLink

ae7e4a1

vercel bot deployed to Preview October 5, 2021 02:24 View deployment

BRKalow approved these changes Oct 5, 2021

View reviewed changes

zchsh merged commit ba93441 into main Oct 5, 2021

zchsh deleted the zs.subnav-dark-theme branch October 5, 2021 03:56

github-actions bot mentioned this pull request Oct 5, 2021

Version Packages #383

Merged

zchsh mentioned this pull request Oct 5, 2021

feat(tabs): add support for dark theme, classNameTabBar #384

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(subnav): add support for dark theme #381

feat(subnav): add support for dark theme #381

zchsh commented Oct 4, 2021 •

edited

Loading

changeset-bot bot commented Oct 4, 2021 •

edited

Loading

vercel bot commented Oct 4, 2021 •

edited

Loading

BRKalow Oct 4, 2021

zchsh Oct 5, 2021

zchsh Oct 5, 2021

zchsh Oct 5, 2021

zchsh Oct 5, 2021

zchsh Oct 5, 2021

zchsh Oct 5, 2021

zchsh Oct 5, 2021

github-actions bot commented Oct 5, 2021 •

edited

Loading

@hashicorp/react-subnav@9.1.0-canary-20219522528

BRKalow commented Oct 5, 2021

zchsh commented Oct 5, 2021

zchsh commented Oct 5, 2021

BRKalow left a comment

		@@ -0,0 +1,43 @@
		/* Theme settings. Note: also consumed in /partials */

feat(subnav): add support for dark theme #381

feat(subnav): add support for dark theme #381

Conversation

zchsh commented Oct 4, 2021 • edited Loading

PR Checklist 🚀

changeset-bot bot commented Oct 4, 2021 • edited Loading

🦋 Changeset detected

vercel bot commented Oct 4, 2021 • edited Loading

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

github-actions bot commented Oct 5, 2021 • edited Loading

📦 Canary Packages Published

@hashicorp/react-subnav@9.1.0-canary-20219522528

BRKalow commented Oct 5, 2021

zchsh commented Oct 5, 2021

zchsh commented Oct 5, 2021

BRKalow left a comment

Choose a reason for hiding this comment

zchsh commented Oct 4, 2021 •

edited

Loading

changeset-bot bot commented Oct 4, 2021 •

edited

Loading

vercel bot commented Oct 4, 2021 •

edited

Loading

github-actions bot commented Oct 5, 2021 •

edited

Loading