Extend `Link` with Styling API, update documentation #1028

lyzadanger · 2023-05-17T18:38:55Z

~~Depends on #1029~~

This PR pilots the styling API proposed in https://docs.google.com/presentation/d/1uTHdWrXKqVuivMI6Z3JR91ZP-Qb_0Ki2fvIAht7JSbk by updating the Link component and a whole lot of related documentation in the pattern library.

There's maybe 50 lines of actual functional change here and a whole bunch of supporting documentation and formatting.

The basic chunks of this work are:

~~Adjustments to some Library components to fix a few glitches (margins, mostly) and add a way to format API props more cleanly~~ (These were chunked off into a separate PR Pattern library: Adjust margins, add Info, InfoItem components #1029)
Updates to Link and its pattern-library documentation
Restructure of the "Using Components" page: Add the API details of the Styling API
Replacement of the "Customizing Components" page with a "Styling Components" page. This covers how to use the Styling API

This work is additive, not breaking.

Testing

I suggest checking out this branch, running make dev and visiting the:

TODOs

Determine appropriate tests to add for this

Future enhancements that would make this better

Auto-generated permalinks and tables of contents would make pattern-library pages easier to navigate

lyzadanger · 2023-05-17T18:39:28Z

src/components/navigation/Link.tsx

+          // underline
+          // TODO: Underline should be controlled by `variant` and should default
+          // to `always`


I want to acknowledge that underline is a problematic prop that doesn't fit into this model neatly. Which means I think it should go in the short future.

My intuition is that a single enum "variant" is probably not going to be enough to accommodate the different "standard"/built-in styles that a component may have. I wouldn't be surprised if we need flag props for example to turn certain elements of the standard style on/off. If the component is unstyled or using a custom variant or size, then these props wouldn't apply though.

lyzadanger · 2023-05-17T18:39:55Z

src/components/navigation/Link.tsx

      {...htmlAttributes}
-      classes={classnames(
-        'rounded-sm',
-        // NB: Base classes are applied by LinkBase


Not anymore. This is a good thing because even this small abstraction caused a regression in one of these components recently.

lyzadanger · 2023-05-17T18:40:09Z

src/pattern-library/components/Library.tsx

@@ -63,20 +63,26 @@ function Heading({
 }: LibraryHeadingProps) {
  if (level <= 2) {
    return (
-      <h2 className="text-3xl text-slate-600 font-bold" {...htmlAttributes}>
+      <h2
+        className="text-3xl text-slate-600 font-bold mb-4"


Margin fixes. If the smattering of changes in this module feel noisy, you might do better reviewing commit by commit.

lyzadanger · 2023-05-17T18:40:22Z

src/pattern-library/components/Library.tsx

@@ -124,7 +130,7 @@ function Section({
        {intro && (
          <div className="text-base space-y-3 leading-relaxed">{intro}</div>
        )}
-        <div className="leading-relaxed">{children}</div>
+        {children}


Removing containing element to fix margins.

lyzadanger · 2023-05-17T18:42:43Z

src/pattern-library/components/patterns/UsingComponentsPage.tsx

+              </Library.InfoItem>
+              <Library.InfoItem label="type">
+                <code>
+                  {'preact.JSX.HTMLAttributes<T extends HTMLElement>'}


I'm not at all convinced I got "types" right in these docs so I'm open to feedback.

I would say this is intuitive enough.

lyzadanger · 2023-05-17T18:42:59Z

src/pattern-library/components/patterns/UsingComponentsPage.tsx

-      >
-        <Library.Pattern title="Props API">
+        <Library.Section
+          id="presentational-components-styling-api"


Added a few IDs by hand in these pages so I could cross link. Hope to automate these soon.

lyzadanger · 2023-05-17T18:43:15Z

src/pattern-library/components/patterns/UsingComponentsPage.tsx

+                disable variant styling.
+              </Library.InfoItem>
+              <Library.InfoItem label="type">
+                <code>{`'custom' | string`}</code>


I'm 99% sure this isn't the way I should express this "type".

acelaya

I like how it looks like 🙂

acelaya · 2023-05-18T09:32:44Z

src/components/navigation/Link.tsx

        {
-          // color
-          'text-brand hover:text-brand-dark': color === 'brand', // default
-          'text-color-text-light hover:text-brand': color === 'text-light',
-          'text-color-text hover:text-brand-dark': color === 'text',
+          'focus-visible-ring rounded-sm': styled,
+          // underline
+          // TODO: Underline should be controlled by `variant` and should default
+          // to `always`
+          'no-underline hover:no-underline': styled && underline === 'none', // default
+          'underline hover:underline': styled && underline === 'always',
+          'no-underline hover:underline': styled && underline === 'hover',
        },


Since styled is a condition for each one of the styles, perhaps we could consider a different formula.

className={classnames( { 'focus-visible-ring rounded-sm': styled, }, styled && { 'no-underline hover:no-underline': underline === 'none', // default 'underline hover:underline': underline === 'always', 'no-underline hover:underline': underline === 'hover', }, // [...] )}

It would allow not having to add the condition every time, but perhaps it makes it less clear, or too subtle. What do you think?

Oh, I like that. I did go look up classnames API and gave it a think, but this syntax didn't occur to me!

codecov · 2023-05-18T13:37:55Z

Codecov Report

Merging #1028 (cbb35e2) into main (9d95fc0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1028   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           53        53           
  Lines          709       715    +6     
  Branches       246       251    +5     
=========================================
+ Hits           709       715    +6

Impacted Files	Coverage Δ
src/components/navigation/LinkBase.tsx	`100.00% <ø> (ø)`
src/components/navigation/Link.tsx	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

robertknight

Generally LGTM. If I understand the intended use of variant, I do suspect we will find that a single variant value may not be enough to express commonly used knobs/switches that still fit within the scope of a "standard style". A component could say that certain props are only applicable when using particular sizes/variants though, so we don't try to make them work if the component is unstyled or using eg. a custom variant.

robertknight · 2023-05-19T11:02:30Z

src/components/navigation/Link.tsx

+          // underline
+          // TODO: Underline should be controlled by `variant` and should default
+          // to `always`


My intuition is that a single enum "variant" is probably not going to be enough to accommodate the different "standard"/built-in styles that a component may have. I wouldn't be surprised if we need flag props for example to turn certain elements of the standard style on/off. If the component is unstyled or using a custom variant or size, then these props wouldn't apply though.

Improve props formatting, deprecate `LinkBase`

Re-structure this page as a style guide for the proposed styling API

acelaya

LGTM

lyzadanger commented May 17, 2023

View reviewed changes

acelaya reviewed May 18, 2023

View reviewed changes

lyzadanger mentioned this pull request May 18, 2023

Pattern library: Adjust margins, add Info, InfoItem components #1029

Merged

lyzadanger force-pushed the styleable-link branch from a25b0f5 to 5fc68e5 Compare May 18, 2023 13:33

lyzadanger changed the base branch from main to pattern-library-improvements May 18, 2023 13:34

Base automatically changed from pattern-library-improvements to main May 18, 2023 13:44

lyzadanger force-pushed the styleable-link branch from 5fc68e5 to bcbdb6c Compare May 18, 2023 13:45

lyzadanger mentioned this pull request May 18, 2023

Consolidate structure of component documentation in Pattern Library #1030

Closed

22 tasks

robertknight reviewed May 19, 2023

View reviewed changes

lyzadanger added 6 commits May 19, 2023 09:09

Update Link component to support styling API

942f6a6

Deprecate LinkBase

33d8991

Update Link documentation for styling API

1a227cd

Improve props formatting, deprecate `LinkBase`

Update Using Components page

be8b40a

Replace "Customizing Components" with "Styling Components"

d93725c

Re-structure this page as a style guide for the proposed styling API

Add basic styling tests to Link

fb6e164

lyzadanger force-pushed the styleable-link branch from bcbdb6c to fb6e164 Compare May 19, 2023 13:24

lyzadanger requested a review from acelaya May 19, 2023 13:24

Make CSS classnames logic more readable

cbb35e2

acelaya approved these changes May 19, 2023

View reviewed changes

lyzadanger merged commit 6119b75 into main May 19, 2023

lyzadanger deleted the styleable-link branch May 19, 2023 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend `Link` with Styling API, update documentation #1028

Extend `Link` with Styling API, update documentation #1028

lyzadanger commented May 17, 2023 •

edited

Loading

lyzadanger May 17, 2023

robertknight May 19, 2023

lyzadanger May 17, 2023

lyzadanger May 17, 2023

lyzadanger May 17, 2023

lyzadanger May 17, 2023

acelaya May 19, 2023

lyzadanger May 17, 2023

lyzadanger May 17, 2023

acelaya left a comment •

edited

Loading

acelaya May 18, 2023 •

edited

Loading

lyzadanger May 18, 2023

codecov bot commented May 18, 2023 •

edited

Loading

robertknight left a comment

robertknight May 19, 2023

acelaya left a comment •

edited

Loading

Extend Link with Styling API, update documentation #1028

Extend Link with Styling API, update documentation #1028

Conversation

lyzadanger commented May 17, 2023 • edited Loading

Testing

TODOs

Future enhancements that would make this better

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

acelaya left a comment • edited Loading

Choose a reason for hiding this comment

acelaya May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 18, 2023 • edited Loading

Codecov Report

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya left a comment • edited Loading

Choose a reason for hiding this comment

Extend `Link` with Styling API, update documentation #1028

Extend `Link` with Styling API, update documentation #1028

lyzadanger commented May 17, 2023 •

edited

Loading

acelaya left a comment •

edited

Loading

acelaya May 18, 2023 •

edited

Loading

codecov bot commented May 18, 2023 •

edited

Loading

acelaya left a comment •

edited

Loading