chore(deps): update btn deps #168

kendallstrautman · 2021-03-12T19:08:24Z

Description

On certain components, the new theming changes aren't working as expected because they are using an old version of the button component. Enter, dependency wormholes....

See call-to-action for an example. Right now the product prop isn't affecting the button color, if you upgrade the the button package, it works as expected. Also, Button didn't seem to actually be affected by dark or light theme values, so I added back in that classname. LMK if that's not wanted, but note that other components depend on this property.

This PR updates all old react-button versions on dependent components and upgrades their implementation if needed. Friction like this makes me think we need to really nail down our dependency approach within this monorepo...there doesn't seem to be a consistent way they are defined between packages.

Packages Affected

button
call-to-action
callouts
casestudy-slider
consent-manager
content-cta
featured-slider
hero
logo-grid
product-downloader
text-split
subnav

Release Information

Please select one (required)

Major (This PR introduces a breaking (incompatible API) change)
Minor (This PR adds functionality but it backwards-compatible)
Patch (This PR adds a backwards-compatible bug fix)

Release Notes (required)

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.
Write a useful description (above) to give reviewers appropriate context.
Add release notes to the appropriate section (above).
Conduct reasonable cross browser testing for both compatibility and responsive behavior (We have a Cross Browser Testing 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.

vercel · 2021-03-12T19:08:26Z

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/BSxuwN1QLwSnj7dT4GEARw5929PG
✅ Preview: https://react-components-git-ksupdate-btn-deps-hashicorp.vercel.app

kendallstrautman · 2021-03-12T19:08:42Z

packages/button/index.js

        `variant-${themeObj.variant}`,
        themeClass,
        { 'brand-neutral': themeObj.brand === 'neutral' },
+        `background-${themeObj.background}`,


Button didn't seem to actually be affected by dark or light theme values, so I added back in that classname. LMK if that's not wanted, but note that other components depend on this property.

kendallstrautman · 2021-03-12T19:09:34Z

packages/call-to-action/index.js

  }
  return (
-    <div className={`g-call-to-action variant-${variant} theme-${theme}}`}>
+    <div className={`g-call-to-action variant-${variant} theme-${theme}`}>


Fix: as the classname wasn't dark} 🤭

kendallstrautman · 2021-03-12T19:10:10Z

packages/callouts/package-lock.json

  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
-    "@hashicorp/react-button": {


I wonder why the updated version of @hashicorp/react-button isn't in the lock file.... 🤔

I think you hit the nail on the head in the comment below - lerna bootstrap modifies package-lock files when it finds and symlinks interdependencies within the monorepo (at least, that's my understanding 😅 )

kendallstrautman · 2021-03-12T19:10:54Z

packages/callouts/package-lock.json

      "version": "1.0.2",
      "resolved": "https://registry.npmjs.org/@hashicorp/react-inline-svg/-/react-inline-svg-1.0.2.tgz",
      "integrity": "sha512-AAFnBslSTgnEr++dTbMn3sybAqvn7myIj88ijGigF6u11eSRiV64zqEcyYLQKWTV6dF4AvYoxiYC6GSOgiM0Yw=="
-    },


I don't know why this was removed...maybe someone didn't run bootstrap after removing that dep?

Yep, this seems accurate! We're tracking when this bites us on the platform board - ideally we'd have a CI check to catch these before they get merged.

Err wait to clarify - I think this would have happened if someone didn't run bootstrap after adding the dep. I know I've forgotten to do that a few times 😅

kendallstrautman · 2021-03-12T19:11:52Z

packages/call-to-action/package.json

  ],
  "dependencies": {
-    "@hashicorp/react-button": "^2.2.4"
+    "@hashicorp/react-button": "^4.1.0"


Are we going to have to update all of these again when we release the breaking change with colors??... I felt like this was something Lerna would take care of, but I guess not?

I'm sort of like, should this just be a peer dep??

I think Lerna should take care of this - I made the mistake of manually bumping versions in recent docs-page work, but as long as there's none of that happening, and as long as the version numbers actually match (as they do in this case thanks to all the work you did 😅 ), I think lerna should do its job.

I don't have a super deep understanding of peer deps vs direct ones, but based on my understanding so far button should be a direct dependency, since we need a specific version of button code to run callouts (rather than callouts just being "compatible" or "intended for use with" a specific version of button).

kendallstrautman · 2021-03-12T19:12:09Z

packages/case-study-slider/index.js

                      </a>
                    </div>
-                    <div className="feature-content g-type-body">
+                    <div


The dark and light theme was broken on this component.

kendallstrautman · 2021-03-12T19:12:37Z

packages/case-study-slider/style.css


      & .slider-frame {
        & .case-study {
+          & .dark {


Text wasn't showing on dark.

kendallstrautman · 2021-03-12T19:12:50Z

packages/logo-grid/index.js

            title={`${c.name} Website`}
            url={c.link}
-            theme="dark-outline"
+            theme={{ variant: 'secondary' }}


Updates to new API

zchsh

✅ Thanks for tackling all the dependency weirdness 💯 🎉

Took a look at the preview and the dark theme fix on case-study-slider & the logo-grid theme format update both look great 🙌

zchsh · 2021-03-12T20:30:24Z

packages/callouts/package-lock.json

      "version": "1.0.2",
      "resolved": "https://registry.npmjs.org/@hashicorp/react-inline-svg/-/react-inline-svg-1.0.2.tgz",
      "integrity": "sha512-AAFnBslSTgnEr++dTbMn3sybAqvn7myIj88ijGigF6u11eSRiV64zqEcyYLQKWTV6dF4AvYoxiYC6GSOgiM0Yw=="
-    },


Yep, this seems accurate! We're tracking when this bites us on the platform board - ideally we'd have a CI check to catch these before they get merged.

zchsh · 2021-03-12T20:31:35Z

packages/callouts/package-lock.json

  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
-    "@hashicorp/react-button": {


I think you hit the nail on the head in the comment below - lerna bootstrap modifies package-lock files when it finds and symlinks interdependencies within the monorepo (at least, that's my understanding 😅 )

zchsh · 2021-03-12T20:32:09Z

packages/call-to-action/package.json

  ],
  "dependencies": {
-    "@hashicorp/react-button": "^2.2.4"
+    "@hashicorp/react-button": "^4.1.0"


I think Lerna should take care of this - I made the mistake of manually bumping versions in recent docs-page work, but as long as there's none of that happening, and as long as the version numbers actually match (as they do in this case thanks to all the work you did 😅 ), I think lerna should do its job.

I don't have a super deep understanding of peer deps vs direct ones, but based on my understanding so far button should be a direct dependency, since we need a specific version of button code to run callouts (rather than callouts just being "compatible" or "intended for use with" a specific version of button).

kendallstrautman · 2021-03-12T21:06:33Z

We're going to ask the Lerna folks about this behavior to get more clarity as to what went wrong here. I'll connect this issue there.

jescalan

Edit: never mind kendall just posted the same thing above!

kendallstrautman · 2021-03-13T00:22:05Z

See the issue, feel free to add more comments or questions! It's kind of an odd one to explain so feel free to chime in.

kendallstrautman · 2021-03-15T14:43:24Z

Thinking outloud: I suppose this PR doesn't necessarily need to go with the branding refresh PR, we could merge and release this separately as a patch once we figure out the dependency stuff.

kendallstrautman · 2021-03-15T18:49:28Z

@jescalan Now that I am actually updating the colors, the legacy button styles are going to get in the way of Visual QA. We may need to review / merge this before we hear back from Lerna or find out exactly what happened.

jescalan · 2021-03-15T22:40:49Z

@kendallstrautman I think that's ok, let's move forward with it then!

accommodates dark background

kendallstrautman · 2021-03-16T18:38:54Z

Cool, I rebased onto the branding assembly. For me, its easier to do one release for the packages with the color updates and this dep stuff, but let me know if we want to merge this into main and release sooner

mwickett · 2021-03-16T18:41:14Z

Because the rollout process is kind of a pain, I vote we avoid doing it twice.

jescalan · 2021-03-16T21:16:24Z

Agreed, let's take that approach 👍🏼

kendallstrautman commented Mar 12, 2021

View reviewed changes

kendallstrautman marked this pull request as ready for review March 12, 2021 19:16

kendallstrautman requested review from brkalow, jescalan, mwickett and zchsh March 12, 2021 19:18

zchsh approved these changes Mar 12, 2021

View reviewed changes

jescalan reviewed Mar 12, 2021

View reviewed changes

kendallstrautman marked this pull request as draft March 12, 2021 21:07

kendallstrautman added the branding project label Mar 15, 2021

kendallstrautman added 5 commits March 16, 2021 11:36

chore(deps): update react-button deps

0868ce9

patch(button): accommodates dark / light background themes

b1f8ba8

patch(case-study-slider): upgrades to latest button api

c293d90

accommodates dark background

patch(call-to-action): fix dark / light theme styles

ad2e893

chore(logo-grid): updates react-button api usage

a3af144

kendallstrautman force-pushed the ks.update-btn-deps branch from 3a7b3c8 to a3af144 Compare March 16, 2021 18:37

kendallstrautman marked this pull request as ready for review March 16, 2021 18:37

kendallstrautman requested a review from jescalan March 16, 2021 18:39

jescalan approved these changes Mar 17, 2021

View reviewed changes

kendallstrautman merged commit 3442622 into ks.assembly-branding-refresh Mar 17, 2021

kendallstrautman deleted the ks.update-btn-deps branch March 17, 2021 17:36

chore(deps): update btn deps #168

chore(deps): update btn deps #168

Uh oh!

Conversation

kendallstrautman commented Mar 12, 2021 • edited by zchsh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Packages Affected

Release Information

PR Checklist 🚀

Uh oh!

vercel bot commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kendallstrautman Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zchsh Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zchsh Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zchsh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zchsh Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kendallstrautman commented Mar 12, 2021

Uh oh!

jescalan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kendallstrautman commented Mar 13, 2021

Uh oh!

kendallstrautman commented Mar 15, 2021

Uh oh!

kendallstrautman commented Mar 15, 2021

Uh oh!

jescalan commented Mar 15, 2021

Uh oh!

kendallstrautman commented Mar 16, 2021

Uh oh!

mwickett commented Mar 16, 2021

Uh oh!

jescalan commented Mar 16, 2021

Uh oh!

Reviewers

Assignees

kendallstrautman commented Mar 12, 2021 •

edited by zchsh

Loading

vercel bot commented Mar 12, 2021 •

edited

Loading

kendallstrautman Mar 12, 2021 •

edited

Loading

zchsh Mar 12, 2021 •

edited

Loading

zchsh Mar 12, 2021 •

edited

Loading

zchsh left a comment •

edited

Loading

zchsh Mar 12, 2021 •

edited

Loading

jescalan left a comment •

edited

Loading