Update AdderToolbar components and styling #4753

lyzadanger · 2022-08-30T17:25:49Z

This PR updates the shared components used by AdderToolbar, and updates its Tailwind styling to eliminate the need for external CSS. I'm focusing on this component because I expect we'll make some adjustments in this area soon for accessibility reasons.

This component has some of the most fiddly styling of any of our components. It was also one of the first I converted to Tailwind. Much has been learned since February, but it doesn't eliminate the reality that the hover behavior for the AdderToolbar is rather nuanced.

These changes:

Bump frontend-shared to 5.4.1 to get the correct AnnotateIcon
Use updated shared components and icons
Refactor styles out of external SASS
- Add a simple Tailwind utility style to apply all: initial, which is something we do for AdderToolbar (it's in a shadow DOM) and isn't available in stock Tailwind
- Add a more complex Tailwind variant to address the very specific hover-styling needs of this component.
Comment the living daylights out of AdderToolbar's styling

This gets us the correct AnnotateIcon

lyzadanger · 2022-08-30T17:27:56Z

src/annotator/components/AdderToolbar.js

-        // its SVG dimensions dictate
-        'h-auto w-auto z-2',
-        'text-grey-3 fill-white',
+    <div


Here's an example of me adding an additional parent component to handle the positioning, and not applying that styling directly to the icon component. This is less of a real concern with icons, which have no classes applied to them other than what you set on className, but it's more hygienic IMHO.

lyzadanger · 2022-08-30T17:29:39Z

src/annotator/components/AdderToolbar.js

+    <ButtonBase
+      classes={classnames(
+        'flex-col gap-y-1 py-2.5 px-2',
+        'text-annotator-sm leading-none',
+        // Default color when the toolbar is not hovered
+        'text-grey-7',
+        // When the parent .group element is hovered (but this element itself is
+        // not), dim this button's text. This has the effect of dimming inactive
+        // buttons.
+        'group-hover:text-grey-5',
+        // When the parent .group element is hovered AND this element is
+        // hovered, this is the "active" button. Intensify the text color, which
+        // will also darken any descendant Icon
+        'hover:group-hover:text-grey-9',
+        // Set the .hover-group class for use by `NumberIcon`
+        'hover-group'


This use case is a good example of both the pros and cons of ButtonBase. On the "pros" side, we automatically get consistent transitions and focus rings and whatnot, so those don't have to be managed manually, nor risk being inconsistent. On the other hand, I just kind of have to "know" that ButtonBase already sets a flex layout, so I just need to set flex-col to go columnar. But I can feel assured that padding and colors won't conflict, as the coalescing convention for base classes is never to style dimensions or colors...

lyzadanger · 2022-08-30T17:30:23Z

src/annotator/components/AdderToolbar.js

      )}
      onClick={onClick}
      title={title}
    >
-      {icon && <Icon classes="text-annotator-lg" name={icon} title={title} />}
+      {Icon && <Icon className="text-annotator-lg" title={title} />}


At least className gives a hint that your classes are the only classes...

lyzadanger · 2022-08-30T17:30:40Z

src/annotator/components/AdderToolbar.js

      dir="ltr"
      style={{
        visibility: isVisible ? 'visible' : 'hidden',
      }}
    >
-      <div className="flex dim-items-on-hover">
+      <div className="flex group">


I wish Tailwind's group class had a better name. It's so...vague.

lyzadanger · 2022-08-30T17:32:13Z

tailwind.config.mjs

+      // Custom variant for a specific use case in which an element needs to
+      // style itself based on the hover states of TWO parents (.group and
+      // .hover-group). See `src/annotator/components/AdderToolbar`
+      addVariant('hover-group-hover', '.group:hover .hover-group:hover &');


I'm not gonna mince words: this is a tad fugly. I hope the comments everywhere help. I've just never quite run into a situation like this before, where the hover state of two ancestors has to be assessed.

A few searches online finds that plenty of folks have found use cases for nested groups, similar to what we have here. It doesn't seem that unusual to me. The basic solutions I've seen boil down to adding variants to create additional groups that are named by semantic function or level.

robertknight

Regarding NumberIcon colors, it looks to me like this particular use case can be solved without needing to deal with nested groups using currentcolor. The need for styling which depends on the states of multiple ancestors doesn't seem that unusual to me (eg. see Tailwind feature request I linked in the comments), and I think it is something we should be able to accomplish without needing a ton of documentation each time. A high-level description of what the user should see is always useful. The solutions I can think of / find online boil down to one of:

Adding some support for labeled groups, with varying degrees of generality, which is basically what you did here. If we're doing this, I'd suggest giving the groups names that reflect their semantic role or level in the hierarchy.
Setting CSS variables at the top of the group, which children can then consume. This allows the ancestors to set the context ("the current foreground color is $X") while individual components encapsulate how it is applied ("set the color property to $X"). currentcolor is a special case of this.

Other than NumberIcon, I suggested we stick with with icon as the prop name to match the pattern library, but continue to accept components as values.

The rest of the changes look good to me.

robertknight · 2022-08-31T09:37:34Z

src/annotator/components/AdderToolbar.js

  );
 }

 /**
 * @param {object} props
 *  @param {number} [props.badgeCount]
- *  @param {string} [props.icon]
+ *  @param {IconComponent} [props.Icon]


We settled on continuing to use the lower-case icon for the prop name in the frontend-shared package. I think we should do the same here.

robertknight · 2022-08-31T10:03:06Z

src/annotator/components/AdderToolbar.js

+        // when its parent button is hovered, even if this badge's span is not
+        // directly hovered.
+        // This is a custom tailwind variant
+        'hover-group-hover:bg-grey-9'


Another way of doing this in plain CSS might be to pass down a custom variable indicating the foreground color. The toolbar would set this to either the unhovered medium gray or dimmed gray depending on its hover state, and the active button could override it. The various elements inside the button would then consume this foreground color variable in different properties (color, background-color etc.). I don't know off-hand how easy that is to translate to Tailwind.

currentcolor could also work here, although we need to introduce a separate element for the background and the text to use the inherited color for the background instead of the inverted color:

function NumberIcon({ badgeCount }) { return ( <span className="rounded px-1 py-0.5 bg-current"> <span className="text-color-text-inverted font-bold">{badgeCount}</span> </span> ); }

robertknight · 2022-08-31T10:18:28Z

src/annotator/components/AdderToolbar.js

+        'text-color-text-inverted font-bold',
+        // Default background color when there is no hovering going on
+        'bg-grey-7',
+        // When the div.group element is hovered, but the .hover-group


The hover-group naming seems ambiguous to me, since both groups (toolbar and button) are being hovered here. The main distinction is either the relative level of the group (more generic) or the semantic meaning of the group (more specific). If the groups had semantic names (eg. toolbar-group or group-toolbar) then you could apply toolbar-group-hover:bg-grey-5, button-group-hover:bg-grey-9 and it would be a bit more obvious what is going on.

You may have already come across this, but I found some ideas in tailwindlabs/tailwindcss#1192.

robertknight · 2022-08-31T10:34:51Z

src/annotator/components/AdderToolbar.js

@@ -110,6 +136,36 @@ function ToolbarButton({ badgeCount, icon, label, onClick, shortcut }) {
 * The toolbar that is displayed above or below selected text in the document,
 * providing options to create annotations or highlights.
 *
+ * The toolbar has nuanced styling for hover. The component structure is:


While this comment is useful, the requirements here don't seem that crazy or weird to me and it is going to be a pain if we need to write this much detail each time. Also there is a risk that the comments get out of date if we revise the implementation to use a simpler method in future.

robertknight · 2022-08-31T10:52:59Z

tailwind.config.mjs

+      // Custom variant for a specific use case in which an element needs to
+      // style itself based on the hover states of TWO parents (.group and
+      // .hover-group). See `src/annotator/components/AdderToolbar`
+      addVariant('hover-group-hover', '.group:hover .hover-group:hover &');


A few searches online finds that plenty of folks have found use cases for nested groups, similar to what we have here. It doesn't seem that unusual to me. The basic solutions I've seen boil down to adding variants to create additional groups that are named by semantic function or level.

lyzadanger · 2022-08-31T12:29:38Z

@robertknight currentColor: keys to the kingdom here! Didn't think of it. Should reduce all of this complexity to minimal. Thanks.

* Use updated components and icons * Refactor to eliminate external styles - Add tailwind utility class for `all: initial`

lyzadanger · 2022-08-31T12:47:22Z

OK, "reboot" here, much simpler, using currentColor, no need for any real cleverness. A single group will do the trick overall.

robertknight

Revised version LGTM

lyzadanger added 2 commits August 30, 2022 12:42

Add all-initial custom Tailwind component-layer class

fbf4687

Bump frontend-shared to 5.4.1

cb2dd35

This gets us the correct AnnotateIcon

lyzadanger commented Aug 30, 2022

View reviewed changes

lyzadanger mentioned this pull request Aug 30, 2022

Convert AdderToolbar to TypeScript #4754

Merged

robertknight reviewed Aug 31, 2022

View reviewed changes

Update AdderToolbar for components, styling

53decc3

* Use updated components and icons * Refactor to eliminate external styles - Add tailwind utility class for `all: initial`

lyzadanger force-pushed the update-adder-toolbar branch from be67125 to 53decc3 Compare August 31, 2022 12:44

lyzadanger requested a review from robertknight August 31, 2022 12:47

robertknight approved these changes Aug 31, 2022

View reviewed changes

lyzadanger mentioned this pull request Aug 31, 2022

Bump @hypothesis/frontend-shared from 5.4.0 to 5.4.1 #4755

Closed

lyzadanger merged commit aaee569 into main Aug 31, 2022

lyzadanger deleted the update-adder-toolbar branch August 31, 2022 13:31

lyzadanger restored the update-adder-toolbar branch August 31, 2022 13:32

lyzadanger deleted the update-adder-toolbar branch January 20, 2023 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update AdderToolbar components and styling #4753

Update AdderToolbar components and styling #4753

lyzadanger commented Aug 30, 2022 •

edited

Loading

lyzadanger Aug 30, 2022

lyzadanger Aug 30, 2022

lyzadanger Aug 30, 2022

lyzadanger Aug 30, 2022

lyzadanger Aug 30, 2022

robertknight Aug 31, 2022

robertknight left a comment

robertknight Aug 31, 2022

robertknight Aug 31, 2022

robertknight Aug 31, 2022

robertknight Aug 31, 2022

robertknight Aug 31, 2022

robertknight Aug 31, 2022

lyzadanger commented Aug 31, 2022

lyzadanger commented Aug 31, 2022

robertknight left a comment

Update AdderToolbar components and styling #4753

Update AdderToolbar components and styling #4753

Conversation

lyzadanger commented Aug 30, 2022 • 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

robertknight 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

lyzadanger commented Aug 31, 2022

lyzadanger commented Aug 31, 2022

robertknight left a comment

Choose a reason for hiding this comment

lyzadanger commented Aug 30, 2022 •

edited

Loading