feat(logo-grid): convert to CSS modules #309

zchsh · 2021-08-28T03:28:54Z

This PR converts @hashicorp/react-logo-grid to CSS modules. One significant change to enable this shift was the removal of Tippy.js, which has been replaced by a ReachUI Dialog element.

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.~~
- Note: test file had only .todos. For now I've left these as-is, have not added new tests yet.
- Rationale for not fixing the test todos yet...
1. component is mostly presentation, and have done a good amount of manual QA
2. I think we would prefer to keep momentum on CSS modules conversions rather than rethink our testing strategy for this component.
- ☝️ That being said, loosely held approach here. Very open to sinking some extra time in to get some good tests written here. (I mean I am open to it either way, just a question of whether we do it now, in this PR, or later, as a follow-up)

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-28T03:28:56Z

🦋 Changeset detected

Latest commit: dda8ed9

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-logo-grid	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-28T03:28:56Z

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/9NtxC3wRbEUJKTiiaJKjCtuynibN
✅ Preview: https://react-components-git-zslogo-grid-css-modules-hashicorp.vercel.app

zchsh · 2021-08-31T19:51:46Z

packages/logo-grid/icons/x.svg

@@ -0,0 +1,3 @@
+<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">


Have replaced × with this icon for design consistency. Adds a bit of weight but felt this was worth it. In the future we'll swap in a Flight icon here instead of using a local .svg file.

zchsh · 2021-08-31T19:52:42Z

packages/logo-grid/index.tsx

+  size?: 'small' | 'medium' | 'large'
+}
+
+function LogoGrid({


I've converted to Typescript, and have done a pretty significant refactor, largely due to the switch from Tippy to ReachUI.

zchsh · 2021-08-31T20:00:01Z

packages/logo-grid/index.tsx

+              [s.removeBorders]: removeBorders,
+            })}
+          >
+            {hasLink ? (


Not super proud of this nested ternary, but it felt like a decent way to avoid prop-drilling. Very open to other ideas on how to clean this up 🙇

Only idea I have would be to make it a separate function component, then you could use normal if/else statements. But this doesn't really feel that bad honestly.

zchsh · 2021-08-31T20:00:31Z

packages/logo-grid/index.tsx

+  )
+}
+
+function CompanyLogo({ company, color, size }) {


Split this component out, similar to in the previous implementation.

zchsh · 2021-08-31T20:02:24Z

packages/logo-grid/partials/dialog-tooltip/index.tsx

+  theme?: 'light' | 'dark'
+}
+
+function DialogTooltip({


Similar to the Reach-based tooltip created for tabs, I split this out in the hopes that it might someday see re-use. I did not try to consolidate with the tabs tooltip - in this case, we have interactive content in our tooltip, so we need to use a more Dialog-like component.

zchsh · 2021-08-31T20:05:14Z

packages/logo-grid/partials/dialog-tooltip/index.tsx

+  /* Odd fix for possible issue with useRect, where
+  tooltipRect did not seem to be set as expected.
+  I think this works because it forces a re-render
+  when "shown" changes, which is exactly when we need
+  to look at the tooltipRef again and re-measure.
+  Without this, tooltipRect seems to always be null.
+  Not actually sure why this works. Need to investigate. */
+  useEffect(setNow, [shown])


I need help understanding this 😅 It feels like useRect should just work without this, and update properly with the dialog's DOMRect when it is opened (and therefore rendered). I'm not sure why it doesn't. Possibly related to Portal use, as I did test the same thing without Portal and it worked fine. cc @BRKalow @jescalan @thiskevinwang in case y'all have any insight 🙇

zchsh · 2021-08-31T20:10:25Z

packages/logo-grid/partials/dialog-tooltip/index.tsx

+        /* We don't want scroll lock, as this is more of a tooltip than a dialog.
+        Due to our tooltip-like positioning, when scroll lock is enabled, the
+        tooltip can become locked in an offscreen position. */
+        dangerouslyBypassScrollLock={true}


On the fence about this. Basically the default & recommended "dialog" behaviour is that scrolling becomes locked when the dialog is visible. In our case thought, I think this is kind of bad - since we're positioning the dialog in a tooltip-like way, it's very possible the dialog will be at least slightly out-of-view and will require scrolling to get to.

An alternate solution I thought of might be to try to auto-scroll the page before we set shown to true, such that the dialog would be ~ centred in the screen. However this seemed like a decent lift, and likely to cause headaches (eg how would we measure the dialog's DOMRect before shown is set?).

I am curious if anyone has any insights on the implications of this disabling. I read through the Reach docs on this topic. Unlike the dangerouslyBypassFocusLock prop, where the downsides seem more clear to me (ie, as a screen reader user it seems like lack of focus lock would really mess things up), I'm unclear on the exact downsides of setting this to false (at least in our use case).

agreed and this seems ok to me in this instance

zchsh · 2021-08-31T20:13:28Z

pages/global.css

@@ -16,7 +16,6 @@
 @import '../packages/toggle/style.css';
 @import '../packages/text-input/style.css';
 @import '../packages/hero/style.css';
-@import '../packages/logo-grid/style.css';


🔪 🎉

This never gets old for me, be forewarned I will probably post these comments in every one of my CSS modules PR self-reviews 🤣

zchsh · 2021-08-31T20:28:44Z

packages/logo-grid/index.tsx

+  integrationLink,
+  removeBorders,
+  size = 'small',
+  className,


I've added a className prop. Rationale here is that a quick SourceGraph search can easily determine whether the old global className was targeted in "overrides" - if so, I immediately add a className prop so that we can, for now, retain these overrides rather than having their replacement block rollout of these components.

In cases where there are no overrides, ie that SourceGraph search comes up empty, I could still see a case for adding a className prop to enable "safe overrides" (as detailed in the Notion doc on CSS). So if I ever find a component that has 0 overrides in the wild, may still be worth adding this 👍

BRKalow

Fantastic documentation, as usual! Left a note about exploring @reach/popover, which I think might be more appropriate for this pattern.

packages/logo-grid/index.tsx

BRKalow · 2021-08-31T20:53:21Z

packages/logo-grid/partials/dialog-tooltip/index.tsx

@@ -0,0 +1,195 @@
+import React, { useState, useRef, useEffect } from 'react'
+import { DialogOverlay, DialogContent } from '@reach/dialog'


I don't mean to throw a wrench in your progress here, but do you think https://www.npmjs.com/package/@reach/popover would be more appropriate for this use case?

This one doesn't appear on their website 🤔

Yeah, not sure why that is exactly. I do think it's the right component for this use though. It looks like they use it as part of their combobox component.

Ah nice - yeah support for the position prop could definitely help clean things up a little and make this feel a little less like a re-implementation of Tippy. Hopefully it'll resolve the annoying useRect issue I was running into as well. Will give it a shot 👍

@BRKalow I gave @reach/popover a shot - did up needing some workarounds, but now have both DialogTooltip and PopoverTooltip with identical APIs which we can choose from. A quick summary:

DialogTooltip uses @reach/dialog, which is well-documented, and aligns somewhat with reach.tech's recommendation to use dialog for "interactive tooltips". By using dialog, we get focus behaviour, keyboard and on-click-outside functionality to dismiss the dialog by default. But, the implementation required significant workarounds, including custom positioning, and a weird effect to force re-rendering whenever the dialog is opened or closed.

PopoverTooltip uses @reach/popover. It seems to have no official documentation - unclear why this is. By using popover, we get focus behaviour and custom positioning by default. But, the implementation required the use of onClickOutside and onFocusOutside to allow the popover to be dismissed as expected when a visitor clicks or focuses away.

Leaning towards using @reach/popover as you recommended, wanted to run this by just in case. If that sounds good, will just commit the deletion of the dialog-based implementation and this should be good to go 👍 🙇

Thanks for spiking this out. The popover implementation seems potentially cleaner...what do you think? It seems better to have to handle the click/focus outside than the custom positioning. I'll let you make the final call here though!

+1 I think popover is a bit cleaner - will go with that unless @jescalan has any strong objections 👍

jescalan

I'm curious about the popover element bryce mentioned - this is GOOD, as expected, but just feels like a lot of manual implementation

.changeset/smart-countries-fly.md

jescalan · 2021-09-01T18:09:57Z

.changeset/smart-countries-fly.md

+- 🔨 Accessibility improvements
+  - grid items with dialog tooltips now render as focus-able `button` elements
+  - tooltip a11y is now improved, with features such as focus lock and auto-focusing of the close button when the dialog is opened, thanks to ReachUI
+- 🔨 Converts to Typescript


jescalan · 2021-09-01T18:17:54Z

packages/logo-grid/index.tsx

+              [s.removeBorders]: removeBorders,
+            })}
+          >
+            {hasLink ? (


Only idea I have would be to make it a separate function component, then you could use normal if/else statements. But this doesn't really feel that bad honestly.

jescalan · 2021-09-01T18:31:35Z

packages/logo-grid/partials/dialog-tooltip/index.tsx

@@ -0,0 +1,195 @@
+import React, { useState, useRef, useEffect } from 'react'
+import { DialogOverlay, DialogContent } from '@reach/dialog'


This one doesn't appear on their website 🤔

jescalan · 2021-09-01T18:50:21Z

packages/logo-grid/partials/dialog-tooltip/index.tsx

+        /* We don't want scroll lock, as this is more of a tooltip than a dialog.
+        Due to our tooltip-like positioning, when scroll lock is enabled, the
+        tooltip can become locked in an offscreen position. */
+        dangerouslyBypassScrollLock={true}


agreed and this seems ok to me in this instance

jescalan · 2021-09-01T18:51:43Z

packages/logo-grid/partials/dialog-tooltip/index.tsx

+      />
+    </Portal>
+  )
+}


My only thoughts here are that this is a lot of complicated custom code - I'm not sure that its avoidable, it almost feels like we removed tippy only to effectively re-implement it manually

…oyance

Co-authored-by: Bryce Kalow <bkalow@hashicorp.com>

Co-authored-by: Jeff Escalante <jescalan@users.noreply.github.com>

zchsh · 2021-09-08T18:54:33Z

@BRKalow @jescalan Last CSS modules component conversion to merge 🎉

Have switched in @reach/popover here as discussed earlier - details in this comment. Open to thoughts here. I'll interpret PR approval as a signal we should go with the popover implementation and drop the dialog one 👍

BRKalow

Happy with Dialog or Popover, though the popover implementation seems a little cleaner to me!

jescalan

I'm good with whichever option you both prefer, which sounds like popover. A little uncomfortable with the amount of custom code involved with both, but it's probably better than tippy, right?

zchsh · 2021-09-08T19:11:44Z

@jescalan Yep, I think it's still more lightweight than Tippy. The "tooltip arrow" part feels like a decent amount of custom code here... I expect if we continue to see tooltip patterns in design (which seems very likely) we can refine things further, and hopefully make the custom parts feel a little more contained 👍

vercel bot had a problem deploying to Preview August 30, 2021 21:29 Failure

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

zchsh force-pushed the zs.logo-grid-css-modules branch from 238fe43 to b655294 Compare August 31, 2021 19:50

vercel bot had a problem deploying to Preview August 31, 2021 19:50 Failure

vercel bot deployed to Preview August 31, 2021 20:12 View deployment

zchsh commented Aug 31, 2021

View reviewed changes

vercel bot deployed to Preview August 31, 2021 20:25 View deployment

zchsh commented Aug 31, 2021

View reviewed changes

zchsh marked this pull request as ready for review August 31, 2021 20:28

zchsh requested review from BRKalow and jescalan August 31, 2021 20:29

BRKalow reviewed Aug 31, 2021

View reviewed changes

jescalan reviewed Sep 1, 2021

View reviewed changes

vercel bot deployed to Preview September 2, 2021 21:29 View deployment

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

zchsh changed the title ~~feat(logo-grid): convert to CSS modules, except Tippy~~ feat(logo-grid): convert to CSS modules Sep 3, 2021

zchsh and others added 10 commits September 8, 2021 13:11

refactor(logo-grid): convert to CSS modules, except Tippy

2ac35c5

feat(logo-grid)!: WIP rip out Tippy, use Reach Dialog instead

490f74e

fix(logo-grid): disable dialog scroll-lock to prevent out-of-view ann…

4183a9f

…oyance

feat(logo-grid): better support dark theme

ed0519d

chore(logo-grid): clean up and style tweaks

371ec12

fix(logo-grid): remove stray import re-introduced from rebase

7226fbe

feat(logo-grid): add support for className prop

d6ef95f

chore(logo-grid): add changeset file

0af90b1

chore(logo-grid): use conventional array type def for data prop

02d27ef

Co-authored-by: Bryce Kalow <bkalow@hashicorp.com>

chore(logo-grid): clarify breaking change imports in changset

1931fce

Co-authored-by: Jeff Escalante <jescalan@users.noreply.github.com>

zchsh force-pushed the zs.logo-grid-css-modules branch from 9d79270 to 1931fce Compare September 8, 2021 17:12

vercel bot deployed to Preview September 8, 2021 17:12 View deployment

refactor(logo-grid): switch from reach/dialog to reach/tooltip

632c785

vercel bot had a problem deploying to Preview September 8, 2021 18:42 Failure

zchsh requested a review from BRKalow September 8, 2021 18:50

fix(logo-grid): fix broken ref type

29e6826

vercel bot deployed to Preview September 8, 2021 18:52 View deployment

zchsh requested a review from jescalan September 8, 2021 18:53

BRKalow approved these changes Sep 8, 2021

View reviewed changes

chore(logo-grid): update changeset to reflect switch to reach/popover

d2af87c

vercel bot deployed to Preview September 8, 2021 18:57 View deployment

jescalan approved these changes Sep 8, 2021

View reviewed changes

chore(logo-grid): clean up unused dialog-tooltip

dda8ed9

vercel bot deployed to Preview September 8, 2021 19:13 View deployment

zchsh merged commit 73927a8 into main Sep 8, 2021

zchsh deleted the zs.logo-grid-css-modules branch September 8, 2021 19:16

github-actions bot mentioned this pull request Sep 8, 2021

Version Packages #327

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logo-grid): convert to CSS modules #309

feat(logo-grid): convert to CSS modules #309

zchsh commented Aug 28, 2021 •

edited

changeset-bot bot commented Aug 28, 2021 •

edited

vercel bot commented Aug 28, 2021 •

edited

zchsh Aug 31, 2021

zchsh Aug 31, 2021

zchsh Aug 31, 2021

jescalan Sep 1, 2021

zchsh Aug 31, 2021

zchsh Aug 31, 2021

zchsh Aug 31, 2021

zchsh Aug 31, 2021

jescalan Sep 1, 2021

zchsh Aug 31, 2021

zchsh Aug 31, 2021

BRKalow left a comment

BRKalow Aug 31, 2021

jescalan Sep 1, 2021

BRKalow Sep 2, 2021

zchsh Sep 2, 2021

zchsh Sep 8, 2021

BRKalow Sep 8, 2021

zchsh Sep 8, 2021

jescalan left a comment

jescalan Sep 1, 2021

jescalan Sep 1, 2021

jescalan Sep 1, 2021

jescalan Sep 1, 2021

jescalan Sep 1, 2021

zchsh commented Sep 8, 2021 •

edited

BRKalow left a comment

jescalan left a comment

zchsh commented Sep 8, 2021

		@@ -0,0 +1,3 @@
		<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">

		@@ -0,0 +1,195 @@
		import React, { useState, useRef, useEffect } from 'react'
		import { DialogOverlay, DialogContent } from '@reach/dialog'

feat(logo-grid): convert to CSS modules #309

feat(logo-grid): convert to CSS modules #309

Conversation

zchsh commented Aug 28, 2021 • edited

PR Checklist 🚀

changeset-bot bot commented Aug 28, 2021 • edited

🦋 Changeset detected

vercel bot commented Aug 28, 2021 • edited

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

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

jescalan 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

zchsh commented Sep 8, 2021 • edited

BRKalow left a comment

Choose a reason for hiding this comment

jescalan left a comment

Choose a reason for hiding this comment

zchsh commented Sep 8, 2021

zchsh commented Aug 28, 2021 •

edited

changeset-bot bot commented Aug 28, 2021 •

edited

vercel bot commented Aug 28, 2021 •

edited

zchsh commented Sep 8, 2021 •

edited