Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a new “JSON CRDT Explorer” site package and updates the shared UI layer to support it (new Drawer component, logo, and provider rename), with corresponding docs and Storybook updates.
Changes:
- Added
@jsonjoy.com/json-crdt-explorer(React + webpack) with Cloudflare Pages (Wrangler) deployment config. - Replaced
NiceUiProvider/useNiceUiServiceswithUiProvider/useUiServicesacross UI + collaborative-ui + docs + Storybook. - Added a new
Drawer(inline/overlay/auto) component and integrated it intoTwoColumnLayout; refreshed UI assets (JsonJoyLogo, Checkbox visuals).
Reviewed changes
Copilot reviewed 66 out of 67 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Adds dependencies for new explorer package and Wrangler/Miniflare toolchain. |
| packages/ui/src/index.ts | Re-exports context API from the UI package root. |
| packages/ui/src/icons/svg/JsonJoyLogo.tsx | Adds a new JsonJoy logo SVG component. |
| packages/ui/src/icons/svg/JsonJoyLogo.stories.tsx | Storybook story for the new logo. |
| packages/ui/src/docs/content/guidelines/getting-started/text.md | Updates docs to use UiProvider. |
| packages/ui/src/docs/App/pages/ThemePage/index.tsx | Renames hook usage to useUiServices. |
| packages/ui/src/docs/App/pages/IconsPage/index.tsx | Renames hook usage to useUiServices. |
| packages/ui/src/docs/App/pages/GuidelinesPage/index.tsx | Renames hook usage to useUiServices. |
| packages/ui/src/docs/App/pages/ComponentsPage/index.tsx | Renames hook usage to useUiServices. |
| packages/ui/src/docs/App/index.tsx | Swaps NiceUiProvider for UiProvider. |
| packages/ui/src/docs/App/Header/Right/ThemeContextItem/index.tsx | Renames hook usage to useUiServices. |
| packages/ui/src/docs/App/Header/Left/index.tsx | Renames hook usage to useUiServices. |
| packages/ui/src/context/UiProvider.tsx | New provider component replacing NiceUiProvider. |
| packages/ui/src/context/services.tsx | Renames useNiceUiServices to useUiServices. |
| packages/ui/src/context/NiceUiProvider.tsx | Keeps legacy entrypoint as a re-export to UiProvider. |
| packages/ui/src/context/index.ts | Updates context exports to the new provider + hook name. |
| packages/ui/src/6-page/TwoColumnLayout/index.tsx | Integrates new overlay drawer behavior on small screens. |
| packages/ui/src/6-page/DocsPages/text.md | Updates docs to use UiProvider. |
| packages/ui/src/5-block/SubNav/ConnectedSubNav.tsx | Renames hook usage to useUiServices. |
| packages/ui/src/5-block/SplitPane/components/Divider.tsx | Removes now-unneeded lint suppression comment. |
| packages/ui/src/5-block/Drawer/types.ts | Adds Drawer type definitions. |
| packages/ui/src/5-block/Drawer/state.ts | Adds a state container for controlled drawers. |
| packages/ui/src/5-block/Drawer/index.ts | Exports Drawer public API surface. |
| packages/ui/src/5-block/Drawer/index.stories.tsx | Storybook coverage for drawer variants. |
| packages/ui/src/5-block/Drawer/hooks/useFocusTrap.ts | Adds modal focus trapping behavior. |
| packages/ui/src/5-block/Drawer/context.ts | Adds context for accessing DrawerState. |
| packages/ui/src/5-block/Drawer/components/OverlayDrawer.tsx | Implements overlay/modal drawer with portal/backdrop. |
| packages/ui/src/5-block/Drawer/components/InlineDrawer.tsx | Implements inline (layout) drawer variant. |
| packages/ui/src/5-block/Drawer/components/DrawerHeader.tsx | Adds a standardized drawer header component. |
| packages/ui/src/5-block/Drawer/components/DrawerFooter.tsx | Adds a standardized drawer footer component. |
| packages/ui/src/5-block/Drawer/components/DrawerControlled.tsx | Chooses inline vs overlay based on mode/breakpoint. |
| packages/ui/src/5-block/Drawer/components/DrawerBody.tsx | Adds a standardized drawer scroll body component. |
| packages/ui/src/5-block/Drawer/components/Drawer.tsx | Top-level Drawer wrapper using DrawerState. |
| packages/ui/src/3-list-item/Breadcrumbs/ConnectedBreadcrumbs.tsx | Renames hook usage to useUiServices. |
| packages/ui/src/2-inline-block/Checkbox/index.tsx | Enhances checkbox visuals with labels and thumb styling. |
| packages/ui/src/2-inline-block/Checkbox/index.stories.tsx | Adds stories for on/off + small variants. |
| packages/ui/src/2-inline-block/BasicButton/index.tsx | Adds comp prop to avoid invalid interactive nesting. |
| packages/json-crdt-explorer/wrangler.toml | Cloudflare Pages deployment configuration. |
| packages/json-crdt-explorer/webpack.config.ts | Webpack config for the explorer app build/dev server. |
| packages/json-crdt-explorer/tsconfig.json | Base TS config for the new package. |
| packages/json-crdt-explorer/tsconfig.build.json | Build TS config (project references, build info). |
| packages/json-crdt-explorer/tsconfig.app.json | App TS config for webpack/ts-loader. |
| packages/json-crdt-explorer/src/Menu/index.tsx | Adds top navigation/menu for the explorer site. |
| packages/json-crdt-explorer/src/main.tsx | App entrypoint rendering UiProvider + App. |
| packages/json-crdt-explorer/src/App.tsx | Explorer app composition (Menu + JsonCrdtExplorer). |
| packages/json-crdt-explorer/src/App.stories.tsx | Storybook story for the explorer app. |
| packages/json-crdt-explorer/SECURITY.md | Security reporting policy for the new package. |
| packages/json-crdt-explorer/README.md | Dev/deploy instructions for the explorer package. |
| packages/json-crdt-explorer/public/index.html | Static HTML template + metadata for the site. |
| packages/json-crdt-explorer/package.json | Package definition + webpack/wrangler scripts. |
| packages/json-crdt-explorer/LICENSE | Adds AGPL-3.0 license text for the package. |
| packages/collaborative-ui/src/JsonCrdtLog/index.tsx | Uses UI styles context to set Paper background. |
| packages/collaborative-ui/src/JsonCrdtExplorer/index.tsx | Adds configurable top offset (removes hardcoded top nav math). |
| packages/collaborative-ui/src/JsonCrdtExplorer/index.stories.tsx | Adjusts story layout to fullscreen. |
| packages/collaborative-ui/src/JsonCrdtExplorer/ExplorerSidenav/index.tsx | Tweaks sidebar copy/layout. |
| packages/collaborative-ui/src/JsonCrdtExplorer/ExplorerMenu/index.tsx | Uses BasicButtonClose comp="span" to avoid invalid nesting. |
| packages/collaborative-ui/src/docs/content/guidelines/getting-started/text.md | Updates docs to use UiProvider. |
| packages/collaborative-ui/src/docs/App/pages/GuidelinesPage/index.tsx | Renames hook usage to useUiServices. |
| packages/collaborative-ui/src/docs/App/pages/ExplorerPage/index.tsx | Passes top into JsonCrdtExplorer. |
| packages/collaborative-ui/src/docs/App/pages/ComponentsPage/index.tsx | Renames hook usage to useUiServices. |
| packages/collaborative-ui/src/docs/App/index.tsx | Swaps NiceUiProvider for UiProvider. |
| packages/collaborative-ui/src/docs/App/Header/Right/ThemeContextItem/index.tsx | Renames hook usage to useUiServices. |
| packages/collaborative-ui/src/docs/App/Header/Left/index.tsx | Renames hook usage to useUiServices. |
| packages/collaborative-ui/src/CopyButton/index.tsx | Splits value/type exports for better TS emit behavior. |
| biome.json | Disables additional a11y lint rules globally. |
| .storybook/preview.ts | Updates Storybook wrapper to use UiProvider. |
| .storybook/main.ts | Adds json-crdt-explorer package to Storybook build. |
Comments suppressed due to low confidence (1)
biome.json:66
- Disabling
a11y/useButtonTypeanda11y/useSemanticElementsglobally reduces protections against common accessibility/behavior issues (e.g., unintended form submission from missingtype, and non-semantic interactive elements). If this was done to unblock a small number of cases, prefer adding targetedbiome-ignorecomments (as is already done elsewhere) so the rules remain effective across the rest of the codebase.
"a11y": {
"noSvgWithoutTitle": "off",
"noStaticElementInteractions": "off",
"useButtonType": "off",
"useSemanticElements": "off"
},
"correctness": {
"noUnusedFunctionParameters": "off",
"noVoidTypeReturn": "off",
| '&:hover': { | ||
| bxsh: `0 0 0 3px ${theme.color.sem.accent[0]}`, | ||
| '& > span': { | ||
| [`& > .${thumbClass}`]: { | ||
| bg: '#f4f4f4', | ||
| 'box-shadow': '0 0 3px rgba(0,0,0,.4)', | ||
| }, |
There was a problem hiding this comment.
thumbClass is used directly inside a CSS selector (& > .${thumbClass}), but rule() class strings in this codebase are typically space-padded and require .trim() when interpolated into selectors (see other usages like .${blockClass.trim()}). Without trimming, the hover rule may not match the thumb element, so the hover styling won’t apply. Use thumbClass.trim() (or otherwise ensure a raw class name) when building the selector.
| }, | ||
| h('span', {style: styleSpan}, ' '), | ||
| h('span', {className: labelClass, style: styleLabelOn, fontFamily: 'monospace', 'aria-hidden': true}, 'I'), | ||
| h('span', {className: labelClass, style: styleLabelOff, fontFamily: 'monospace', 'aria-hidden': true}, 'O'), | ||
| h('span', {className: thumbClass, style: styleSpan}, ' '), |
There was a problem hiding this comment.
fontFamily is being passed as a DOM attribute on the <span> elements rather than as part of the style object. This won’t apply the font and can produce React “unknown prop” warnings. Move fontFamily: 'monospace' into the style object (or into the class).
| side: rest.side, | ||
| width: typeof rest.width === 'number' ? rest.width : 260, | ||
| }); | ||
| }, [_state, rest.side]); | ||
| const open = state.open$.use(); | ||
| const width = state.width$.use(); | ||
| const side = state.side$.use(); | ||
| const handleOpenChange = React.useCallback( |
There was a problem hiding this comment.
The drawer state is created from defaultOpen and rest.width, but the useMemo dependency list only includes _state and rest.side. If defaultOpen or width props change across renders (and no external state is supplied), the internal DrawerState will not reflect the new values. Include the relevant props in the dependency list or update the existing state in an effect instead of recreating it selectively.
| const siblings = Array.from(document.body.children).filter((ch) => ch !== el && !ch.contains(el)); | ||
| siblings.forEach((s) => ((s as HTMLElement).inert = true)); | ||
|
|
||
| const firstFocusable = el.querySelector<HTMLElement>( |
There was a problem hiding this comment.
useFocusTrap only computes siblings from document.body.children. When the drawer is portaled into a custom mountNode (supported by OverlayDrawer), the mountNode itself will be excluded because it contains the panel, leaving the rest of the app inside mountNode non-inert (i.e., still interactive) even though the drawer is modal. Consider deriving the inert scope from the portal container/root (or also inerting mountNode’s siblings/children appropriately) so mountNode portals remain truly modal.
| const SvgFF: React.FC<Props> = ({size = 36, color = false}) => { | ||
| return ( | ||
| <svg width={size} viewBox="0 0 92 57" fill="none"> | ||
| <path |
There was a problem hiding this comment.
Only width is set on the <svg> element; other SVG icons in this codebase set both width and height (or explicit style sizing). Without height, the rendered size can be inconsistent across browsers/layouts. Consider setting height as well (e.g., height={size} or a proportional height based on the viewBox).
| <nanoTheme.Provider theme={theme ?? (theme2 === 'dark' ? 'dark' : 'light')}> | ||
| <StylesProvider dark={theme2 === 'dark'}> | ||
| <Kbd> |
There was a problem hiding this comment.
When theme prop is provided, nanoTheme.Provider uses it, but StylesProvider still derives dark from services.theme$. This can put the theme providers out of sync (e.g., forced light theme while StylesProvider stays dark), leading to inconsistent styling. Derive StylesProvider’s dark from the same resolved theme used for nanoTheme.Provider (or update services.theme$ when theme is overridden).
| const logo = ( | ||
| <BasicTooltip nowrap renderTooltip={() => 'jsonjoy.com'}> | ||
| <BasicButton to="https://jsonjoy.com" round target="_blank" rel="noreferrer" style={linkStyle} size={40}> | ||
| <span style={{margin: '0 1px 0 -1px', display: 'flex'}}> | ||
| <JsonJoyLogo color size={24} /> | ||
| </span> | ||
| </BasicButton> | ||
| </BasicTooltip> | ||
| ); |
There was a problem hiding this comment.
Links opened with target="_blank" should include rel="noopener noreferrer" to prevent window.opener vulnerabilities. Currently rel="noreferrer" is used; consider adding noopener explicitly (or ensure the underlying BasicButton/Link always applies it when target=_blank).
No description provided.