feat(Demo Site): DS Demo Site: Pt. 2 #161

jmuzina · 2025-03-20T02:25:49Z

Done

Depends on #160 , #177

Builds first major section of the DS demo site.

Includes:

Showcase configuration, controls (adjust styles for a showcase using a tooltip form)
Showcase CSS export (copy CSS variables from a showcase to the clipboard)
Showcase navigation, use prev/next buttons to move between showcases
Showcase state management - each showcase has its own configuration state

What's next (future PRs):

PT3 - Form integration - consume form components from the forms package, dynamically generate setting controls
PT4 - Applying styling to the typographic specimen example
PT5 - SSR handling - integrating TanStack router with SSR

QA

Open the Showcase
Hover the "Configure" button and play around with settings. See that the example updates styles live.
Use the "Copy" button to copy settings as CSS variables.
Use "Prev" and "Next" buttons to switch between examples.

Open

PR readiness check

PR should have one of the following labels:
- Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
PR title follows the Conventional Commits format.
All packages define the required scripts in package.json:
- All packages: check, check:fix, and test.
- Packages with a build step: build.

advl

It looks very promising. A few comments so far :

I don't see ./routeTree.gen.js - How is it generated - what is its role ?
I wonder if this app would benefit from having a storybook as well ?
Curious about your approach to the shadow dom. We can ofc discuss it later today :)

jmuzina · 2025-03-25T17:17:30Z

This doesn't yet work with SSR - I suspect some changes will need to be made to follow Tanstack's SSR instructions:
https://tanstack.com/router/latest/docs/framework/react/guide/ssr

I see that as something that can be tackled in another PR.

jmuzina · 2025-03-25T17:23:09Z

* I don't see `./routeTree.gen.js` - How is it generated - what is its role ?

@advl

It is generated in the build stage using the Vite Tanstack Router plugin.

Per the TanStack router docs, it is imported by the application at runtime and used to instantiate the TanStack router.

* I wonder if this app would benefit from having a storybook as well ?

Good call, I've added a storybook.

* Curious about your approach to the shadow dom. We can ofc discuss it later today :)

As discussed yesterday, this is currently using react-shadow to wrap examples inside of the shadow DOM. All examples are isolated from the styles of the rest of the site.

As a potential improvement, we could do this natively with Web Components at a later date.

apps/react/demo/.storybook/preview.ts

advl

Good :

Usage of the reducer
having not refactored too much your code - this helps with api flexibility and speed of development. We can refactor later
good API for the examples and type safety
Overall the architecture solves the problem and demonstrates you have a wide understanding of what needs to be solved, matching your ownership

Could be better :

File-based separation of concerns (see comments), in general having one file per function helps.
Folder structure and naming in some cases
Avoid usage of bem - use modern css instead ;)

Feel free to break this down into smaller chuks if it helps with the review.

Please keep notes of how you would split the next stages of implementation, and if there are some items you would like to look at later. This will help me understand your train of though as well, and ensure we have a future-proof view over quality.

.github/workflows/chromatic.de-demo-site.yml

apps/react/demo/biome.json

advl · 2025-03-25T19:47:07Z

apps/react/demo/src/routes/__root.tsx

is this a tanstack naming convention ? Is it possible to capture it somehow ?

Yes, this comes from TanStack naming conventions.

Possibly we can reference this in ARCHITECTURE.md.

advl · 2025-03-25T19:47:50Z

apps/react/demo/src/routes/about.tsx

Could we possibly split the route declaration and the component ? What are the recommended patterns for that ?

The format you are seeing here is the recommended pattern: https://tanstack.com/router/latest/docs/framework/react/quick-start

I have adapted this slightly for the Showcase route, where the entire route content is encapsulated in a separate component (ref). This creates a better distinction between route and component and also allows me to test the entire page with Chromatic.

I would recommend to have two files, one for the route declaration, one for the Component, unless there are sizeable downsides to that

apps/react/demo/src/ui/Showcase/Showcase.tsx

advl · 2025-03-25T20:03:56Z

apps/react/demo/src/ui/Showcase/common/ExampleControls/Context.tsx

This file is where I have the most issues with in your code.

You should separate it into code units that are well differentiated :
helper functions in utils folder, for instanceutils/generateCssVariables.ts reexported in utils/index.ts
hooks in the hooksfolder
Context in Context.js
Provider in Provider.js

Look at the example from launchpad here, which is the best implementation of those patterns so far.

Good point - I've had a go at following that pattern, have a look and LMK what you think.

It's much better. There are a few things here and there I'll take a longer look tomorrow

apps/react/demo/src/ui/Showcase/common/ExampleControls/ExampleControls.tsx

advl · 2025-03-25T20:06:20Z

apps/react/demo/src/ui/Showcase/common/ExampleControls/ExampleControls.tsx

+    (event: ChangeEvent<HTMLSelectElement>) => {
+      if (activeExampleName) {
+        dispatch({
+          type: "UPDATE_SETTING",


Would this be a good use case for an enum ?

Currently it is a strictly typed literal - it must be UPDATE_SETTING or RESET_EXAMPLE.

Do you think it would be more beneficial to use an enum?

I usually like them but the benefit here is not clear.

I think they reducers can be a good use case but it's mostly an opinion here

apps/react/demo/src/ui/Showcase/common/ExampleControls/ExampleControls.tsx

advl · 2025-03-25T20:09:05Z

apps/react/demo/src/ui/Showcase/common/ExampleControls/types.ts

you will likely want to separate those types in the reducer-related types, that could live alongside it in the same folder, and the component related types. Also we are using TPrefixType for parameters - while it is ok, it is a different pattern from what is in use in the form, so we might want to discuss it together and find our future pattern

I will give a more in depth type review once I can see more clearly the domains they relate to - the file structure would help

I have separated reducer/context/provider and component-related types.

Regarding Type prefixes, I have a habit of using a T prefix myself (it dates back to college days) but will drop t for the sake of project consistency.

I think the domains should be more clear in the folder structure now.

I don't mind the T if we're able to justify it as part of the future style guide. Happy to discuss it more later

advl

A few more comments - not final, but to save time tomorrow

advl · 2025-03-25T22:34:11Z

.github/workflows/chromatic.ds-demo-site.yml

If the same of the project is "demo" then this should be "demo" as well. Otherwise you should rename the application folder name as well to be "ds-demo-site"

advl · 2025-03-25T22:37:08Z

apps/react/demo/src/ui/Example/common/Controls/useExampleControls/useExampleControls.tsx

+      handleLineHeightChange,
+    }),
+    [
+      handleFontFamilyChange,


Dependencies can be simplified to activeExample or activeExampleName

advl · 2025-03-25T22:38:43Z

apps/react/demo/src/ui/Example/common/index.ts

@@ -0,0 +1,3 @@
+export * from "./Controls/index.js";
+export * from "./Renderer/index.js";
+export * from "./examples/index.js";


If those have been moved to data then they can be removed from here

advl · 2025-03-25T22:40:36Z

apps/react/demo/src/ui/Example/utils/generateCssVariables.ts

+    const setting = settings[settingName as keyof AllExampleSettings];
+    if (!setting || setting.disableFormats?.css || setting.value === undefined)
+      continue;
+    const cssVarName = `--${casing.toKebabCase(settingName)}`;


This somehow seems counterintuitive to me. I imagine it makes more sense to write css in css and, if necessary to transform it into js.

advl · 2025-03-25T22:41:40Z

apps/react/demo/src/ui/Showcase/Showcase.tsx

+
+const Showcase = () => {
+  return (
+    <Example examples={SHOWCASE_EXAMPLES}>


I suggest to rename examples to "items" so when a map is required in a component we use a predictable name. Curious what you think here would be best

advl reviewed Mar 20, 2025

View reviewed changes

jmuzina force-pushed the ds-demo-site branch from d690e64 to a9c1105 Compare March 20, 2025 21:06

jmuzina mentioned this pull request Mar 25, 2025

chore: CP boilerplate for demo site #177

Merged

jmuzina force-pushed the ds-demo-site branch from a9c1105 to 8e36bda Compare March 25, 2025 15:10

jmuzina changed the title ~~wip: DS Demo Site: Pt. 1~~ feat(Demo Site): DS Demo Site: Pt. 1 Mar 25, 2025

jmuzina added Review: Code needed Review: QA needed Feature 🎁 Review: Chromatic Needed Requires visual testing review labels Mar 25, 2025

jmuzina changed the title ~~feat(Demo Site): DS Demo Site: Pt. 1~~ feat(Demo Site): DS Demo Site: Pt. 2 Mar 25, 2025

jmuzina commented Mar 25, 2025

View reviewed changes

apps/react/demo/.storybook/preview.ts Outdated Show resolved Hide resolved

jmuzina force-pushed the ds-demo-site branch 4 times, most recently from 55a4cc1 to 4256d1f Compare March 25, 2025 19:04

jmuzina marked this pull request as ready for review March 25, 2025 19:09

jmuzina requested a review from advl March 25, 2025 19:26

advl requested changes Mar 25, 2025

View reviewed changes

jmuzina force-pushed the ds-demo-site branch from a045e5b to 6fea167 Compare March 25, 2025 21:49

jmuzina added 2 commits March 25, 2025 18:11

Demo site part 2

85711ab

Follow up on code review feedback

c0b7049

jmuzina force-pushed the ds-demo-site branch from 6fea167 to c0b7049 Compare March 25, 2025 22:12

advl reviewed Mar 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Demo Site): DS Demo Site: Pt. 2 #161

feat(Demo Site): DS Demo Site: Pt. 2 #161

jmuzina commented Mar 20, 2025 •

edited

Loading

advl left a comment

jmuzina commented Mar 25, 2025

jmuzina commented Mar 25, 2025 •

edited

Loading

advl left a comment

advl Mar 25, 2025

jmuzina Mar 25, 2025

advl Mar 25, 2025

jmuzina Mar 25, 2025

advl Mar 25, 2025

advl Mar 25, 2025

jmuzina Mar 25, 2025

advl Mar 25, 2025

advl Mar 25, 2025

jmuzina Mar 25, 2025

advl Mar 25, 2025

advl Mar 25, 2025

advl Mar 25, 2025

jmuzina Mar 25, 2025

advl Mar 25, 2025

advl left a comment

advl Mar 25, 2025

advl Mar 25, 2025

advl Mar 25, 2025

advl Mar 25, 2025

advl Mar 25, 2025

feat(Demo Site): DS Demo Site: Pt. 2 #161

Are you sure you want to change the base?

feat(Demo Site): DS Demo Site: Pt. 2 #161

Conversation

jmuzina commented Mar 20, 2025 • edited Loading

Done

QA

PR readiness check

advl left a comment

Choose a reason for hiding this comment

jmuzina commented Mar 25, 2025

jmuzina commented Mar 25, 2025 • edited Loading

advl 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

advl 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

jmuzina commented Mar 20, 2025 •

edited

Loading

jmuzina commented Mar 25, 2025 •

edited

Loading