Add the beginnings of a Preact create-group form #8789

seanh · 2024-07-02T13:33:48Z

Add our modern frontend stack (TypeScript, Preact, Tailwind, frontend-shared) and add a feature-flagged re-implementation of the create-group form using this modern frontend stack.

To add the new frontend-stack I copied how it has been done in Via, and then just fixed up any issues I ran into.

The new create-group form uses the same HTML and CSS as the legacy form, but is not yet functional.

The new create-group form doesn't use any Tailwind CSS classes or anything from frontend-shared, but these things have been added so it could use them. I'm not sure what direction we'll go in the end: continue to use the legacy CSS classes directly, as in this PR? Re-implement the legacy CSS classes as Tailwind-style classes and frontend-shared components? Break with the legacy style and have the new form look different?

Testing

Log in and go to http://localhost:5000/groups/new, you should still see the legacy create-group form and it should function as normal.
Log in as devdata_admin, go to http://localhost:5000/admin/features, enable the preact_create_group_form feature flag, then go to http://localhost:5000/groups/new. You should see a form that looks almost exactly like the legacy create-group form but isn't functional yet.

seanh · 2024-07-02T13:36:05Z

.babelrc

+  "presets": [
+    "@babel/preset-typescript",
+    [
+      "@babel/preset-react",
+      {
+        "runtime": "automatic",
+        "importSource": "preact"
+      }
+    ],
+    "@babel/preset-env"
+  ],
+  "env": {
+    "development": {
+      "presets": [
+        [
+          "@babel/preset-react",
+          {
+            "development": true,
+            "runtime": "automatic",
+            "importSource": "preact"
+          }
+        ]
+      ]
+    }
+  }


This is copied from Via: https://github.com/hypothesis/via/blob/2f9e78063cb0e4fc731915f2f4c5f791da1c7467/.babelrc#L1-L34

Via also passes "bugfixes": true to preset-env but that seems separate from the issue of adding our Preact + Tailwind stack so I've not added that.

I'm fine if you want to make that a separate PR, although I think this is a pretty safe option to add as we've been using it a long time. The documentation mentions that it will become the default option in future Babel releases, so we probably ought to be using it everywhere.

Related to this, we need to get our browserslist updated everywhere to something a bit more modern.

Done: c204e69

seanh · 2024-07-02T13:42:57Z

h/static/scripts/tsconfig.json

+{
+  "compilerOptions": {
+    "allowJs": true,
+    "allowSyntheticDefaultImports": true,
+    "checkJs": true,
+    "lib": ["es2018", "dom"],
+    "jsx": "react-jsx",
+    "jsxImportSource": "preact",
+    "module": "commonjs",
+    "noEmit": true,
+    "strict": true,
+    "target": "ES2020",
+    "useUnknownInCatchVariables": false
+  },
+  "include": ["**/*.js", "**/*.ts", "**/*.tsx"],
+  "exclude": [
+    // Tests and test infrastructure
+    "**/test/*.js",
+    "bootstrap.js",
+    "karma.config.cjs"
+  ]
+}


Copied from Via: https://github.com/hypothesis/via/blob/2f9e78063cb0e4fc731915f2f4c5f791da1c7467/via/static/scripts/tsconfig.json#L1-L23

seanh · 2024-07-02T13:46:31Z

tailwind.config.js

+import tailwindConfig from '@hypothesis/frontend-shared/lib/tailwind.preset.js';
+
+export default {
+  presets: [tailwindConfig],
+  content: [
+    './h/static/scripts/**/*.{js,ts,tsx}',
+    './node_modules/@hypothesis/frontend-shared/lib/**/*.js',
+  ],
+  theme: {
+    extend: {
+      animation: {
+        'fade-in': 'fade-in 0.3s forwards',
+        'fade-out': 'fade-out 0.3s forwards',
+        'slide-in-from-right': 'slide-in-from-right 0.3s forwards ease-in-out',
+      },
+      fontFamily: {
+        sans: [
+          '"Helvetica Neue"',
+          'Helvetica',
+          'Arial',
+          '"Lucida Grande"',
+          'sans-serif',
+        ],
+      },
+      boxShadow: {
+        // Similar to tailwind's default `shadow-inner` but coming from the
+        // right edge instead of the left
+        'r-inner': 'inset -2px 0 4px 0 rgb(0,0,0,.05)',
+      },
+      keyframes: {
+        'fade-in': {
+          '0%': {
+            opacity: '0',
+          },
+          '100%': {
+            opacity: '1',
+          },
+        },
+        'fade-out': {
+          '0%': {
+            opacity: '1',
+          },
+          '100%': {
+            opacity: '0',
+          },
+        },
+        'slide-in-from-right': {
+          '0%': {
+            opacity: '0',
+            left: '100%',
+          },
+          '80%': {
+            left: '-10px',
+          },
+          '100%': {
+            left: '0',
+            opacity: '1',
+          },
+        },
+      },
+    },
+  },
+};


Copied from Via's tailwind.config.js. I'm not sure if we need the theme stuff in h

seanh · 2024-07-03T07:42:18Z

gulpfile.js

 );

 gulp.task('watch-css', () => {
  gulp.watch(
-    'h/static/styles/**/*.scss',
+    ['h/static/styles/**/*.{css,scss}', 'h/static/scripts/**/*.{js,ts,tsx}'],


Same as in Via: https://github.com/hypothesis/via/blob/2f9e78063cb0e4fc731915f2f4c5f791da1c7467/gulpfile.js#L21-L30

seanh · 2024-07-03T08:20:51Z

package.json

+    "@babel/preset-react": "^7.24.7",
+    "@babel/preset-typescript": "^7.24.7",


All the packages I'm adding are copied from Via's package.json. I set the minimum version numbers of all the packages I added to the current latest versions on NPM, which means some of them are newer than the minimum versions given in Via, may as well start h off on the latest versions (I don't think this actually makes any difference to the compiled requirements anyway).

seanh · 2024-07-03T11:25:09Z

h/views/groups.py

+@view_defaults(
+    route_name="group_create",
+    renderer="h:templates/groups/create.html.jinja2",
+    is_authenticated=True,
+    feature="preact_create_group_form",
+)
+class GroupCreateController:
+    def __init__(self, request):
+        self.request = request
+
+    @view_config(request_method="GET")
+    def get(self):
+        """Render the page for creating a new group."""
+        return {}


Starting fresh with a completely new controller for the new version of the page.

seanh · 2024-07-03T11:26:29Z

h/assets.ini

+
+create_group_form_css =
+  styles/create-group-form.css
+
+create_group_form_js =
+  scripts/create-group-form.bundle.js


Adding the asset bundles for my new "create group form" app, same as Via does for its video player app: https://github.com/hypothesis/via/blob/2f9e78063cb0e4fc731915f2f4c5f791da1c7467/via/assets.ini#L3-L7

I expect we'll want to make this a bit more general in future so we have a bundle for all the new UI rather than just the "create group" UI. Only once that gets large enough would we then need to split it up again.

seanh · 2024-07-03T11:29:40Z

h/templates/groups/create.html.jinja2

+  <div id="create-group-form"></div>
+{% endblock %}


Replacing the legacy form with our Preact app. The group.html.jinja2 base template renders this inside a couple of container divs (<div class="content paper"><div class="form-container">):

h/h/templates/layouts/group.html.jinja2

Lines 9 to 15 in b29dd69

{% block content %}

<div class="content paper">

<div class="form-container">

{% include "h:templates/includes/flash-messages.html.jinja2" %}

{{ self.page_content() }}

</div>

</div>

It also adds flash messages to the page, which we might not want here..

seanh · 2024-07-03T11:30:31Z

h/templates/groups/legacy_create.html.jinja2

+{% extends "h:templates/layouts/group.html.jinja2" %}
+
+{% set page_title = 'Create a new private group' %}
+
+{% block page_content %}
+  <h1 class="form-header">Create a new private group</h1>
+  {{ form }}
+
+  <footer class="form-footer">
+    {# This form has at least one required field. If that changes we should update this footer. #}
+    <span class="form-footer__required">
+      <span class="form-footer__symbol">
+      *
+      </span>
+      <span class="form-footer__text">
+        Required
+      </span>
+    </span>
+  </footer>
+{% endblock page_content %}


This is the previous create.html.jinja2 template, just renamed

seanh · 2024-07-03T12:33:34Z

h/static/scripts/create-group-form.tsx

+      <h1 class="form-header">Create a new private group</h1>
+
+      <form class="form group-form__form">
+        <div class="form-input">
+          <label for="name" class="form-input__label group-form__name-label">
+            Name <span class="form-input__required">*</span>
+          </label>
+          <input
+            type="text"
+            id="name"
+            class="form-input__input group-form__name-input has-label"
+            autofocus=""
+            autocomplete="off"
+            required
+          />
+          <span class="form-input__character-counter is-ready">0/25</span>
+        </div>
+
+        <div class="form-input">
+          <label
+            class="form-input__label group-form__description-label"
+            for="description"
+          >
+            Description
+          </label>
+          <textarea
+            id="description"
+            class="form-input__input group-form__description-input has-label"
+          ></textarea>
+          <span class="form-input__character-counter is-ready">0/250</span>
+        </div>
+
+        <div class="form-actions">
+          <div class="u-stretch"></div>
+
+          <div class="form-actions__buttons">
+            <button class="form-actions__btn btn primary-action-btn group-form__submit-btn">
+              Create group
+            </button>
+          </div>
+        </div>
+      </form>
+
+      <footer class="form-footer">
+        <span class="form-footer__required">
+          <span class="form-footer__symbol">*</span>
+          <span class="form-footer__text">Required</span>
+        </span>
+      </footer>


I've copied the HTML elements and CSS classes from the legacy create group form.

We're still including the legacy CSS (the site_css bundle) because the parts of the legacy page surrounding the new group form are still in use (the Hypothesis logo in the top left, the HTML <head> with various things in it, and the <body> and some layout/container elements). Therefore, we can just use the legacy CSS classes from our Preact component!

This is a quick way to get a create group form that looks exactly like the legacy one.

It's very close but this doesn't end up looking exactly like the legacy form: the inclusion of @tailwind base messes with the layout, changing some of the sizes/margins/paddings just slightly. This can be fixed by simply removing Tailwind (which is not being used).

Whether we actually want to go with this look in the end or not, I don't know yet.

robertknight

This looks good. There are a few things I noticed that can be simplified or updated. Regarding the TypeScript setup, I posted a minimal diff so you can get yarn typecheck running locally. Feel free to do that now or it can be addressed after. We should get it running in CI soon though.

robertknight · 2024-07-03T14:06:21Z

.babelrc

+  "presets": [
+    "@babel/preset-typescript",
+    [
+      "@babel/preset-react",
+      {
+        "runtime": "automatic",
+        "importSource": "preact"
+      }
+    ],
+    "@babel/preset-env"
+  ],
+  "env": {
+    "development": {
+      "presets": [
+        [
+          "@babel/preset-react",
+          {
+            "development": true,
+            "runtime": "automatic",
+            "importSource": "preact"
+          }
+        ]
+      ]
+    }
+  }


I'm fine if you want to make that a separate PR, although I think this is a pretty safe option to add as we've been using it a long time. The documentation mentions that it will become the default option in future Babel releases, so we probably ought to be using it everywhere.

Related to this, we need to get our browserslist updated everywhere to something a bit more modern.

robertknight · 2024-07-03T14:08:41Z

gulpfile.js

+  buildCSS(
+    [
+      './node_modules/bootstrap/dist/css/bootstrap.css',
+      './h/static/styles/admin.scss',
+      './h/static/styles/help-page.scss',
+      './h/static/styles/site.scss',
+      './h/static/styles/vendor/icomoon.css',
+      './h/static/styles/create-group-form.css',
+    ],
+    { tailwindConfig },
+  ),


Indeed. I would probably separate out the build for the Tailwind and non-Tailwind CSS. You can easily add two separate gulp tasks (build-legacy-css, build-new-css) and then make a build-css task which runs them in parallel. See the gulpfile.js in the client repo for an example of doing something like this.

robertknight · 2024-07-03T14:09:50Z

h/assets.ini

+
+create_group_form_css =
+  styles/create-group-form.css
+
+create_group_form_js =
+  scripts/create-group-form.bundle.js


I expect we'll want to make this a bit more general in future so we have a bundle for all the new UI rather than just the "create group" UI. Only once that gets large enough would we then need to split it up again.

robertknight · 2024-07-03T14:13:26Z

h/static/scripts/create-group-form.tsx

@@ -0,0 +1,63 @@
+import { render } from 'preact';


For Preact components the convention is to name the modules after the name of the main component and put them in a components directory (with additional hierarchy as needed). Non-component modules use a kebab-case naming convention. Was this something you planned to do as part of making the UI actually functional?

Done. I went with:

h/static/scripts/group-forms/index.tsx

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

h/static/styles/group-forms.css

assets.ini bundle names: group_forms_css and group_forms_js

robertknight · 2024-07-03T14:13:50Z

h/static/scripts/tsconfig.json

+    "allowJs": true,
+    "allowSyntheticDefaultImports": true,
+    "checkJs": true,
+    "lib": ["es2018", "dom"],


This can safely be bumped to es2020 at least.

Done: 7633256

robertknight · 2024-07-03T14:16:10Z

h/static/scripts/tsconfig.json

+    "lib": ["es2018", "dom"],
+    "jsx": "react-jsx",
+    "jsxImportSource": "preact",
+    "module": "commonjs",


The module configuration differs across repos. We probably should be using es2020 everywhere, which is the configuration in the client. If making that change doesn't cause any errors, I'd do it now, otherwise we can revisit later.

Done: f7f6b97

robertknight · 2024-07-03T14:18:41Z

h/static/scripts/tsconfig.json

+    "target": "ES2020",
+    "useUnknownInCatchVariables": false
+  },
+  "include": ["**/*.js", "**/*.ts", "**/*.tsx"],


Suggested change

"include": ["**/*.js", "**/*.ts", "**/*.tsx"],

"include": ["**/*.ts", "**/*.tsx"],

Limit type checking to TS / TSX files for now. That will make it easier to turn on typechecking in CI. You can also remove the exclude list as well.

Done: 44f0507

robertknight · 2024-07-03T14:23:52Z

package.json

-    "checkformatting": "prettier --check *.js h/static/scripts",
-    "format": "prettier --list-different --write *.js h/static/scripts",
+    "checkformatting": "prettier --check *.{js,ts,tsx} h/static/scripts",
+    "format": "prettier --list-different --write *.{js,ts,tsx} h/static/scripts",
    "lint": "eslint h/static/scripts",
    "test": "gulp test"


To run TS, we'd add a script like:

"typecheck": "tsc --build h/static/scripts/tsconfig.json"

To make that command run successfully there are a few minor corrections needed. The most notable one is that there are some things in JSX that are slightly different from HTML, and TS can catch these for you.

diff --git a/h/static/scripts/create-group-form.tsx b/h/static/scripts/create-group-form.tsx index 4393d90de..f858a8ca4 100644 --- a/h/static/scripts/create-group-form.tsx +++ b/h/static/scripts/create-group-form.tsx @@ -14,8 +14,8 @@ function CreateGroupForm() { type="text" id="name" class="form-input__input group-form__name-input has-label" - autofocus="" - autocomplete="off" + autoFocus + autoComplete="off" required /> <span class="form-input__character-counter is-ready">0/25</span> @@ -57,7 +57,7 @@ function CreateGroupForm() { } function init() { - render(<CreateGroupForm />, document.querySelector('#create-group-form')); + render(<CreateGroupForm />, document.querySelector('#create-group-form')!); } init(); diff --git a/h/static/scripts/tsconfig.json b/h/static/scripts/tsconfig.json index 33fd708ba..2aadc5b43 100644 --- a/h/static/scripts/tsconfig.json +++ b/h/static/scripts/tsconfig.json @@ -12,7 +12,7 @@ "target": "ES2020", "useUnknownInCatchVariables": false }, - "include": ["**/*.js", "**/*.ts", "**/*.tsx"], + "include": ["**/*.ts", "**/*.tsx"], "exclude": [ // Tests and test infrastructure "**/test/*.js", diff --git a/package.json b/package.json index 4a8fac0ce..224c038c4 100644 --- a/package.json +++ b/package.json @@ -9,7 +9,8 @@ "checkformatting": "prettier --check *.{js,ts,tsx} h/static/scripts", "format": "prettier --list-different --write *.{js,ts,tsx} h/static/scripts", "lint": "eslint h/static/scripts", - "test": "gulp test" + "test": "gulp test", + "typecheck": "tsc --build h/static/scripts/tsconfig.json" }, "dependencies": { "@babel/core": "^7.24.7", diff --git a/yarn.lock b/yarn.lock index 3848796d9..7ec5987e2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2705,12 +2705,12 @@ __metadata: linkType: hard "@types/vinyl@npm:^2.0.9": - version: 2.0.11 - resolution: "@types/vinyl@npm:2.0.11" + version: 2.0.12 + resolution: "@types/vinyl@npm:2.0.12" dependencies: "@types/expect": ^1.20.4 "@types/node": "*" - checksum: e921718775992ada7712a26da9635fb970bcdbcf4c4cc832344092d68710ffaf7dd3cce42b0050b2741b2cf75a30f3e1b87e1032ae26552db1faac17094d2b50 + checksum: 885b7813676eed755b94d42501a56bf36c8d93d8ef6cc4d495bb5779fc5c81ffbcee7d0defe77515a90bf46209968da84b559471611efa0d2900ec75fa1a84de languageName: node linkType: hard

The changes in yarn.lock are the result of removing the existing entry for a package (@types/vinyl) that was triggering errors and re-running yarn to add the latest version.

Done: 6c5ccec

robertknight · 2024-07-03T14:26:47Z

tailwind.config.js

+  ],
+  theme: {
+    extend: {
+      animation: {


The fade-in, fade-out and slide-in-from-right animations are now part of our component library so we should be able to remove them here and in Via. I would remove everything related to animations from here in this PR. If we find there is anything missing in the shared library we can add them there. The animations are used by the ToastMessages component.

Done: 013c74f

acelaya

No further comments apart from what Rob already mentioned.

This is to make way for adding a new `GroupCreateController` in future.

Add the non-functional beginnings of a new, feature-flagged re-implementation of the create-group page as a Preact app.

Same as we do in Via

The legacy CSS build task doesn't need the Tailwind config.

Same as in client

This should make it easier to turn on type-checking

These are now in our component library so not needed here.

seanh force-pushed the add-preact branch 10 times, most recently from adb082c to ad65d76 Compare July 3, 2024 12:35

seanh changed the title ~~Add our modern frontend stack~~ Add the beginnings of a Preact re-implementation of the create-group form Jul 3, 2024

seanh commented Jul 3, 2024

View reviewed changes

seanh requested review from robertknight and acelaya July 3, 2024 12:43

seanh changed the title ~~Add the beginnings of a Preact re-implementation of the create-group form~~ Add the beginnings of a Preact create-group form Jul 3, 2024

seanh marked this pull request as ready for review July 3, 2024 12:51

robertknight self-assigned this Jul 3, 2024

robertknight approved these changes Jul 3, 2024

View reviewed changes

acelaya approved these changes Jul 3, 2024

View reviewed changes

seanh force-pushed the add-preact branch from ad65d76 to 83eb202 Compare July 3, 2024 16:21

seanh added 8 commits July 4, 2024 11:17

Rename GroupCreateController to LegacyGroupCreateController

ed09c32

This is to make way for adding a new `GroupCreateController` in future.

Add beginnings of feature-flagged Preact create group page

5b1947c

Add the non-functional beginnings of a new, feature-flagged re-implementation of the create-group page as a Preact app.

Add "bugfixes": true to present-env config

c204e69

Same as we do in Via

Separate legacy and Tailwind CSS build tasks

2d9fa27

The legacy CSS build task doesn't need the Tailwind config.

Bump es2018 TypeScript compiler option to es2020

7633256

Use es2020 TypeScript module config instead of CommonJS

f7f6b97

Same as in client

Only type-check TypeScript files for now

44f0507

This should make it easier to turn on type-checking

Add TypeScript type-checking to make sure and CI

6c5ccec

seanh force-pushed the add-preact branch from 779a0f2 to 2bddce5 Compare July 4, 2024 10:22

Remove animations settings from Tailwind config

013c74f

These are now in our component library so not needed here.

Re-run yarn

c4e97d0

seanh force-pushed the add-preact branch from 2bddce5 to c4e97d0 Compare July 4, 2024 10:24

seanh merged commit 663b223 into main Jul 4, 2024
9 checks passed

seanh deleted the add-preact branch July 4, 2024 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the beginnings of a Preact create-group form #8789

Add the beginnings of a Preact create-group form #8789

seanh commented Jul 2, 2024 •

edited

Loading

seanh Jul 2, 2024

robertknight Jul 3, 2024

seanh Jul 4, 2024

seanh Jul 2, 2024

seanh Jul 2, 2024

seanh Jul 3, 2024

seanh Jul 3, 2024 •

edited

Loading

seanh Jul 3, 2024

seanh Jul 3, 2024

robertknight Jul 3, 2024

seanh Jul 3, 2024 •

edited

Loading

seanh Jul 3, 2024

seanh Jul 3, 2024 •

edited

Loading

robertknight left a comment

robertknight Jul 3, 2024

robertknight Jul 3, 2024

robertknight Jul 3, 2024

robertknight Jul 3, 2024

seanh Jul 4, 2024

robertknight Jul 3, 2024

seanh Jul 4, 2024

robertknight Jul 3, 2024

seanh Jul 4, 2024

robertknight Jul 3, 2024

seanh Jul 4, 2024

robertknight Jul 3, 2024

seanh Jul 4, 2024

robertknight Jul 3, 2024

seanh Jul 4, 2024

acelaya left a comment

		"@babel/preset-react": "^7.24.7",
		"@babel/preset-typescript": "^7.24.7",

	{% block content %}
	<div class="content paper">
	<div class="form-container">
	{% include "h:templates/includes/flash-messages.html.jinja2" %}
	{{ self.page_content() }}
	</div>
	</div>

	"include": ["*/.js", "*/.ts", "*/.tsx"],
	"include": ["*/.ts", "*/.tsx"],

Add the beginnings of a Preact create-group form #8789

Add the beginnings of a Preact create-group form #8789

Conversation

seanh commented Jul 2, 2024 • edited Loading

Testing

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

seanh Jul 3, 2024 • 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

seanh Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Jul 3, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

seanh commented Jul 2, 2024 •

edited

Loading

seanh Jul 3, 2024 •

edited

Loading

seanh Jul 3, 2024 •

edited

Loading

seanh Jul 3, 2024 •

edited

Loading