feat(hero)!: convert to CSS modules #299

zchsh · 2021-08-28T00:42:43Z

🎟️ Asana Task
🔍 Preview Link

This PR updates @hashicorp/react-hero to use 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-08-28T00:42:46Z

🦋 Changeset detected

Latest commit: 68c8004

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

This PR includes changesets to release 2 packages

Name	Type
@hashicorp/react-text-input	Minor
@hashicorp/react-hero	Major

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-08-28T00:42:46Z

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/5u56SUEYWUNvsH78sfbheVN3jB7t
✅ Preview: https://react-components-git-zshero-css-modules-hashicorp.vercel.app

zchsh · 2021-09-02T18:46:38Z

packages/hero/props.js

@@ -226,6 +226,10 @@ module.exports = {
    description: 'Whether or not to center the hero content',
    testValue: false,
  },
+  className: {


Added support for className, in case it's needed to deal with potential g-hero overrides.

zchsh · 2021-09-02T19:00:55Z

.changeset/long-crabs-change.md

+'@hashicorp/react-text-input': minor
+---
+
+- ✨ Adds support for a className prop.


Adding className support to text-input was necessary to avoid a :global override selector previously used in hero.

zchsh · 2021-09-02T19:05:55Z

packages/hero/index.test.js

+
+const defaultProps = getTestValues(props)
+
+it('should add a provided className to the root element', () => {


I added the className prop in this PR, so felt right to add a corresponding test.

zchsh · 2021-09-02T19:06:22Z

packages/text-input/index.js

  const error = form.touched[field.name] && form.errors[field.name]
  //  Label htmlFor relies on an id on the input field, which must be
  //  unique to prevent collisions between fields or forms on the same page
  const inputId = 'u' + uuidv1()
  return (
    <div
-      className="g-text-input"
+      className={classNames('g-text-input', className)}


Necessary to support hero-related styling.

zchsh · 2021-09-02T19:06:39Z

pages/global.css

@@ -15,7 +15,6 @@
 @import '../packages/consent-manager/style.css';
 @import '../packages/toggle/style.css';
 @import '../packages/text-input/style.css';
-@import '../packages/hero/style.css';


BRKalow

👏

BRKalow · 2021-09-02T20:06:36Z

packages/hero/carousel.js

+              className={classnames(s.videoWrapper, {
+                [s.isActive]: this.state.active === i,
+                [s.isDeactivating]: this.state.deactivating === i,
+              })}


BRKalow · 2021-09-02T20:07:17Z

packages/hero/carousel.module.css

+  position: relative;
+  transform: translateX(-60px);
+  line-height: 0;
+  box-shadow: 0 14.3254px 14.3254px rgba(37, 41, 55, 0.16);


Wow those are some crazy fractions!

Yeah i'ma clean these up. As mentioned in other CSS comments, just flat out copy-pasted old styles for consistency, but this is pretty distracting 👍

BRKalow · 2021-09-02T20:08:03Z

packages/hero/carousel.module.css

+
+  &.isActive {
+    opacity: 1;
+    padding-top: calc((100% * 0.63569) + 28px);


curious where this value came from 💭

I'll be honest, I have no idea - have just ported over old styles. Also curious, so I'll try to figure it out and add some comments.

✅ Okay so it's aspect ratio related - ensures there's no layout shift as the video loads. I added a clarifying comment here, and added a note on expected aspect ratio to props.js 👍

BRKalow · 2021-09-02T20:08:14Z

packages/hero/carousel.module.css

+
+.bar {
+  align-items: center;
+  background: #0e1016;


Do we have color vars for this?

Kind of - here's as-is, with the current hard-coded HEX values:

Closest var-friendly alternative with --black and --gray-1:

Definitely slightly different, but could definitely see the case for switching. I do worry a little about tying this tweak in with the modules change - even though it feels super minor, I think there are cases where it the difference would be noticeable, eg if the BG is full --black. So going to leave as-is for now, but definitely on board with rooting out HEX values in the future 👍

BRKalow · 2021-09-02T20:08:18Z

packages/hero/carousel.module.css

+  width: 100%;
+
+  & span {
+    background: #252937;


Do we have color vars for this?

Forgot to follow-up here - details in #299 (comment)

BRKalow · 2021-09-02T20:19:54Z

packages/hero/index.js

+        /* Note: has-videos is only used in Percy on www-next. Should likely be removed */
+        { 'has-videos': hasVideos },


Removing this now 👍

zchsh changed the base branch from main to zs.button-css-modules August 28, 2021 03:22

Base automatically changed from zs.button-css-modules to main August 31, 2021 14:46

zchsh added 2 commits September 2, 2021 14:32

refactor(hero)!: convert to CSS modules

892cc59

fix(hero): fix repeated selectors for overrides

841b605

zchsh force-pushed the zs.hero-css-modules branch from 0c90d04 to 841b605 Compare September 2, 2021 18:32

vercel bot deployed to Preview September 2, 2021 18:32 View deployment

feat(hero): support className prop

00a8540

vercel bot deployed to Preview September 2, 2021 18:40 View deployment

chore(hero): add changeset file

ebd1a06

vercel bot deployed to Preview September 2, 2021 18:42 View deployment

chore(hero): update changeset file

69a8179

vercel bot deployed to Preview September 2, 2021 18:43 View deployment

fix(text-input): document support for className prop

7772ac0

vercel bot deployed to Preview September 2, 2021 18:55 View deployment

fix(hero): change line order for diff clarity

ed79702

vercel bot deployed to Preview September 2, 2021 19:02 View deployment

fix(hero): clean up classNames

ba864b7

vercel bot deployed to Preview September 2, 2021 19:07 View deployment

zchsh commented Sep 2, 2021

View reviewed changes

zchsh requested a review from a team September 2, 2021 19:08

zchsh marked this pull request as ready for review September 2, 2021 19:08

BRKalow approved these changes Sep 2, 2021

View reviewed changes

jescalan approved these changes Sep 3, 2021

View reviewed changes

feat(hero): hide video progress-bar from Percy by default

6482418

vercel bot deployed to Preview September 3, 2021 20:44 View deployment

chore(hero): remove Percy-only global className

3120e51

vercel bot deployed to Preview September 3, 2021 20:49 View deployment

chore(hero): update changeset with note on has-videos

c972ebd

vercel bot deployed to Preview September 3, 2021 20:50 View deployment

docs(hero): add comment on video aspect magic number

68c8004

vercel bot deployed to Preview September 3, 2021 21:34 View deployment

zchsh merged commit bc36b72 into main Sep 7, 2021

zchsh deleted the zs.hero-css-modules branch September 7, 2021 15:00

This was referenced Sep 7, 2021

Version Packages #327

Merged

Version Packages zchsh/react-components#1

Open

zchsh mentioned this pull request Oct 14, 2021

fix: fixups related to CSS modules rollout #335

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hero)!: convert to CSS modules #299

feat(hero)!: convert to CSS modules #299

zchsh commented Aug 28, 2021 •

edited

Loading

changeset-bot bot commented Aug 28, 2021 •

edited

Loading

vercel bot commented Aug 28, 2021 •

edited

Loading

zchsh Sep 2, 2021

zchsh Sep 2, 2021

zchsh Sep 2, 2021

zchsh Sep 2, 2021

zchsh Sep 2, 2021

BRKalow left a comment

BRKalow Sep 2, 2021

BRKalow Sep 2, 2021

zchsh Sep 3, 2021

BRKalow Sep 2, 2021

zchsh Sep 3, 2021

zchsh Sep 3, 2021

BRKalow Sep 2, 2021

zchsh Sep 3, 2021

BRKalow Sep 2, 2021

zchsh Sep 7, 2021

BRKalow Sep 2, 2021

zchsh Sep 3, 2021

zchsh Sep 3, 2021


		const defaultProps = getTestValues(props)

		it('should add a provided className to the root element', () => {

		/* Note: has-videos is only used in Percy on www-next. Should likely be removed */
		{ 'has-videos': hasVideos },

feat(hero)!: convert to CSS modules #299

feat(hero)!: convert to CSS modules #299

Conversation

zchsh commented Aug 28, 2021 • edited Loading

PR Checklist 🚀

changeset-bot bot commented Aug 28, 2021 • edited Loading

🦋 Changeset detected

vercel bot commented Aug 28, 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

BRKalow left a 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

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

zchsh commented Aug 28, 2021 •

edited

Loading

changeset-bot bot commented Aug 28, 2021 •

edited

Loading

vercel bot commented Aug 28, 2021 •

edited

Loading