Style the new create-group-form #8797

seanh · 2024-07-05T17:04:50Z

Style the new create-group-form to look similar to the legacy one, but using Tailwind for CSS.

This involves a lot of hard-coded values that we'll probably want to replace.

seanh · 2024-07-05T17:07:43Z

h/static/scripts/group-forms/components/CreateGroupForm.tsx

@@ -3,27 +3,52 @@ import { Button, Input, Textarea } from '@hypothesis/frontend-shared';
 export default function CreateGroupForm() {
  return (
    <>
-      <h1>Create a new private group</h1>
+      <h1 class="mt-[55px] mb-[30px] text-[#3f3f3f] text-[19px] leading-[15px]">


Note the font-size: 19px; line-height: 15px;. This is necessary if we want the vertical position of the heading to exactly match the legacy form. But having a line-height that's smaller than the font-size is non-sensical and looks accidental in the legacy code (the line-height is inherited from CSS applied to the <body> element).

seanh · 2024-07-05T17:10:17Z

h/static/scripts/group-forms/components/CreateGroupForm.tsx

+          <label for="name" class="text-[#7a7a7a] text-[13px] leading-[15px]">
+            Name<span class="text-[#d00032]">*</span>
+          </label>


The sequence of classes text-[#7a7a7a] text-[13px] leading-[15px] is repeated a few times, and the text-[#d00032] (for the *) is repeated a couple of times. This probably suggests that there's a sub-component I should factor out here. The sub-component may not be <Label> though since this CSS class sequence is also used for the character counters and for the <footer> at the bottom of the form neither of which are <label>'s. So I'm not sure what the name for it is

seanh · 2024-07-05T17:14:11Z

h/static/scripts/group-forms/components/CreateGroupForm.tsx

+          <div class="flex">
+            <div class="grow"></div>


This flexbox stuff for right-aligning something is repeated a few times as well. Not sure if the duplication is worthy of an attempt to factor it out though.

robertknight · 2024-07-08T08:59:31Z

A few general things about JSX and Preact components:

For historical reasons, JSX conventionally uses different names than HTML for some attributes: className instead of class and htmlFor instead of for. Both the JSX and HTML names work in Preact, but the JSX ones are standard
Empty elements should self-close in JSX, like XML but unlike HTML: <div className="foo"/> instead of <div className="foo"></div>
To generate element IDs that are globally unique, use the useId hook:

import { useId } from 'preact/hooks';

function Widget() {
  const fooId = useId();
  return <>
    <label htmlFor={fooId}>Some field</label>
    <input id={fooId}/>
  </>
}

Components are cheap in Preact, both in terms of runtime overhead and boilerplate in the code. Creating small "helper" components to avoid duplication is common practice. For example, the character counter could be extracted like so:

function CharCounter({ value, limit }: { value: number; limit: number }) {
  return (
    <div className="flex">
      <div className="grow" />
      <span className="text-grey-6">
        {value}/{limit}
      </span>
    </div>
  );
}
...
<CharCounter value={0} limit={250}/>

Some notes about styling:

Flexbox will automatically wrap inline children (text, spans etc.) in boxes, so you can omit the unstyled divs around the right-aligned elements.
I think we can "snap" the colors to nearby colors from our palette quite easily: text-brand for red, text-grey-6 for labels / counter / footnote and also the bottom divider, text-grey-7 for the heading. The component library uses more variety for heading sizes and colors than I think it should, so we'll need to tweak that.
For font sizes, I think our existing body fonts have tended to feel a little small. I would suggest text-sm for all the body text and text-xl for the heading. text-sm works out to be 14px by default, which is also GitHub's standard body text size. This is one pixel larger than the 13px that hypothes.is currently uses.

seanh · 2024-07-10T10:49:19Z

Ok, pushed various fixups to this:

Replaced class with className
Replaced for with htmlFor
Used self-closing empty tags, e.g. <div /> instead of <div></div>.
Used useId() to generate the IDs for the form fields
Removed duplication by extracting <Label />, <CharacterCounter /> and <Star /> components
Removed unnecessary unstyled <div>'s in flexbox layouts
Replaced arbitrary colors, font sizes, line heights, margins and paddings with nearest-matching Tailwind classes
Added a containing <div> and moved some CSS classes onto it in order to deduplicate them
Made the group description <textarea> large enough to hold 250 chars

How it looks now:

Font-sizes and line-heights

Heading font-size: the legacy page uses a font-size of 19px, so use Tailwind's text-xl which gives a font-size of 20px (there's no default Tailwind class matching 19px exactly).

Heading line-height: the legacy page uses a line-height of of 15px for the heading, which is smaller than the font-size and causes lines to overlap with each other if a heading wraps onto multiple lines. This line-height may be an accident: the heading gets the line-height from a CSS rule applied to the <body> element. Use Tailwind's /none which gives a line-height of 1 (which works out as 20px, the same as the font-size). We could use /3 instead (which would give a line-height of 12px), this would be closer to the legacy line-height (13px) but I feel that the line-height should be at least equal to the font-size otherwise headings look bad when they wrap onto multiple lines, so I've chosen the smallest line-height in Tailwind's scale that is at least equal to the font-size.

Label font-size: the legacy page uses 13px for the form element labels, button text, and footer. Tailwind's font-size scale has no class that matches 13px exactly. It has either text-xs (12px) which I feel would be too small (the legacy 13px is already probably too small, we don't want to go even smaller) or text-sm (14px). So use text-sm as the closest match that is at least as large as the legacy font-size.

Label line-height: the legacy page uses a 15px (roughly 1.15) line-height for the form element labels, button text, and footer. Use Tailwind's /tight (1.25, 17.5px) as the closest matching leading in Tailwind's scale.

Margins and paddings

I've arbitrarily chosen to go with the nearest Tailwind classes that are at least as large as the legacy margins and paddings they're replacing, not smaller. An earlier commit did the same with font-size's and line-height's.

Group description `<textarea>` height

The legacy form's group description <textarea> isn't large enough to hold the maximum length of 250 chars:

I've added h-24 to the new <textarea> to make it large enough:

robertknight

I had a few small recommendations, but otherwise this looks great. I expect we will refine some of the styling details as we build out more forms.

h/static/scripts/group-forms/components/CreateGroupForm.tsx

robertknight · 2024-07-10T10:59:22Z

h/static/scripts/group-forms/components/CreateGroupForm.tsx

+        <div className="mb-4">
+          <Label
+            htmlFor={descriptionId}
+            text={'Description'}


Suggested change

text={'Description'}

text="Description"

Same with the "Name" label. Booleans and string literals are the only types that have special syntax. For everything else use braces.

An alternative API here would be to have the label content be passed as the children. This is mainly useful if we needed to support more complex content than a string.

import type { ComponentChildren } from 'preact'; function Label({ children, required }: { children: ComponentChildren, required?: boolean }) { return <label ...>{children}{required ? <Star/> : null}</label> } <Label ...>Description</Label>

robertknight · 2024-07-10T11:13:48Z

h/static/scripts/group-forms/components/CreateGroupForm.tsx

-          <label for="name">Name*</label>
-          <Input id="name" autofocus autocomplete="off" required />
-          0/25
+        <div className="mb-4">


I think in future we're going to want to tweak the label or the field container so there is a bit more spacing between the bottom of the label and the top of the input field. I suggest to leave as-is in this PR and we can tweak it afterwards. This might involve creating a reusable "form field container" component to handle this.

We may want to replace examples like "Input with label" and "Textarea with label" in the component library.

I'd currently find it hard to decide how much spacing to put between a label and its form field, without any vertical rhythm or grid (that I'm aware of) it just seems like an arbitrary choice. Perhaps a defined grid that all components stick to is something we're missing?

Style the new create-group-form to look similar to the legacy one, but using Tailwind for CSS. This involves a lot of hard-coded values that we'll probably want to replace.

`class` works just fine but `className` is conventional.

`for` works just fine but `htmlFor` is conventional.

In the new create-group form replace the arbitrary `font-size` and `line-height` values (which were copied from the legacy forms) with values from Tailwind's default `font-size` and `line-height` scales. Heading `font-size`: the legacy page uses a `font-size` of `19px`, so use Tailwind's `text-xl` which gives a `font-size` of `20px` (there's no default Tailwind class matching `19px` exactly). Heading `line-height`: the legacy page uses a `line-height` of of `15px` for the heading, which is smaller than the `font-size` and causes lines to overlap with each other if a heading wraps onto multiple lines. This `line-height` may be an accident: the heading gets the `line-height` from a CSS rule applied to the `<body>` element. Use Tailwind's `/none` which gives a `line-height` of `1` (which works out as `20px`, the same as the `font-size`). We could use `/3` instead (which would give a `line-height` of `12px`), this would be closer to the legacy `line-height` (`13px`) but I feel that the `line-height` should be at least equal to the `font-size` otherwise headings look bad when they wrap onto multiple lines, so I've chosen the smallest `line-height` in Tailwind's scale that is at least equal to the `font-size`. Label `font-size`: the legacy page uses `13px` for the form element labels, button text, and footer. Tailwind's `font-size` scale has no class that matches `13px` exactly. It has either `text-xs` (`12px`) which I feel would be too small (the legacy `13px` is already probably too small, we don't want to go even smaller) or `text-sm` (`14px`). So use `text-sm` as the closest match that is at least as large as the legacy `font-size`. Label `line-height`: the legacy page uses a `15px` (roughly `1.15`) `line-height` for the form element labels, button text, and footer. Use Tailwind's `/tight` (`1.25`, `17.5px`) as the closest matching leading in Tailwind's scale.

Deduplicate some CSS classes by adding them to a containing `<div>`.

In the new create-group form, replace arbitrary margins and paddings (which were copied from the legacy form) with the nearest matching default Tailwind classes. I've arbitrarily chosen to go with the nearest Tailwind classes that are at least as large as the legacy margins and paddings they're replacing, not smaller. An earlier commit did the same with `font-size`'s and `line-height`'s.

Large enough to hold the maximum length of 250 chars.

The convention in our .tsx files is to put the main/exported component at the bottom and internal components or other helpers above.

seanh requested a review from robertknight July 5, 2024 17:04

seanh commented Jul 5, 2024

View reviewed changes

seanh marked this pull request as draft July 10, 2024 09:19

seanh marked this pull request as ready for review July 10, 2024 10:49

robertknight approved these changes Jul 10, 2024

View reviewed changes

seanh added 18 commits July 10, 2024 13:35

Style the new create-group-form

f35984f

Style the new create-group-form to look similar to the legacy one, but using Tailwind for CSS. This involves a lot of hard-coded values that we'll probably want to replace.

Change class to className in JSX

84933b5

`class` works just fine but `className` is conventional.

Change for to htmlFor in JSX

b140395

`for` works just fine but `htmlFor` is conventional.

Formatting

659d983

Use self-closing empty tags in JSX

59fcce2

Use Preact's useId hook

97be7db

Extract CharacterCounter component

e4dc924

Extract <Label/> component

6771ed9

Remove some unnecessary <div>'s

dbd170e

Use colors from our Tailwind theme

2f3f7b5

Deduplicate some CSS classes

70f96c6

Deduplicate some CSS classes by adding them to a containing `<div>`.

Extract <Star/> component

6615ef5

Make the group description <textarea> larger

b1f89ec

Large enough to hold the maximum length of 250 chars.

Small refactor

80e9a69

Re-order components in CreateGroupForm.txt

c0e290b

The convention in our .tsx files is to put the main/exported component at the bottom and internal components or other helpers above.

Small refactor

917b48e

seanh force-pushed the style-preact-create-group-form branch from 413b866 to 917b48e Compare July 10, 2024 12:36

seanh merged commit 4b867f7 into main Jul 10, 2024
9 checks passed

seanh deleted the style-preact-create-group-form branch July 10, 2024 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style the new create-group-form #8797

Style the new create-group-form #8797

seanh commented Jul 5, 2024 •

edited

Loading

seanh Jul 5, 2024

seanh Jul 5, 2024 •

edited

Loading

seanh Jul 5, 2024

robertknight commented Jul 8, 2024

seanh commented Jul 10, 2024

robertknight left a comment

robertknight Jul 10, 2024

seanh Jul 10, 2024

robertknight Jul 10, 2024

seanh Jul 10, 2024

Style the new create-group-form #8797

Style the new create-group-form #8797

Conversation

seanh commented Jul 5, 2024 • edited Loading

seanh Jul 5, 2024

Choose a reason for hiding this comment

seanh Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Jul 5, 2024

Choose a reason for hiding this comment

robertknight commented Jul 8, 2024

seanh commented Jul 10, 2024

Font-sizes and line-heights

Margins and paddings

Group description <textarea> height

robertknight left a comment

Choose a reason for hiding this comment

robertknight Jul 10, 2024

Choose a reason for hiding this comment

seanh Jul 10, 2024

Choose a reason for hiding this comment

robertknight Jul 10, 2024

Choose a reason for hiding this comment

seanh Jul 10, 2024

Choose a reason for hiding this comment

seanh commented Jul 5, 2024 •

edited

Loading

seanh Jul 5, 2024 •

edited

Loading

Group description `<textarea>` height