projects/events-cash-register
Watch
5
Fork
You've already forked events-cash-register
 0
Code Issues 1 Pull requests 1 Packages 1 Activity Actions

feat: major ui refactor #97

Merged
davide merged 3 commits from feat/ui into main 2026-07-16 15:44:34 +00:00
Conversation 1 Commits 3 Files changed 93 +1146 -765
davide commented 2026-07-16 14:19:27 +00:00
Owner
Copy link
No description provided.
feat: major ui refactor
Some checks failed
check-build.yaml / Check that project builds (pull_request) Failing after 1s
Details
check-commitizen.yaml / Check the PR title (pull_request) Failing after 1s
Details
check-formatter.yaml / Checks with formatters (pull_request) Failing after 1s
Details
check-linter.yaml / Checks with linters (pull_request) Failing after 1s
Details
check-syntax.yaml / Checks with static tools (pull_request) Has been cancelled
Details
check-yamllint.yaml / Lint yaml files (pull_request) Has been cancelled
Details
forgejo-agent / LLM Bot response generation (pull_request) Failing after 1s
Details
ae9c267b2d
Merge branch 'main' into feat/ui
Some checks failed
check-build.yaml / Check that project builds (pull_request) Successful in 1m22s
Details
check-formatter.yaml / Checks with formatters (pull_request) Failing after 12s
Details
check-linter.yaml / Checks with linters (pull_request) Failing after 18s
Details
check-syntax.yaml / Checks with static tools (pull_request) Successful in 23s
Details
check-yamllint.yaml / Lint yaml files (pull_request) Successful in 4s
Details
b615ad95ba
chore: fix lints
All checks were successful
check-build.yaml / Check that project builds (pull_request) Successful in 1m7s
Details
check-formatter.yaml / Checks with formatters (pull_request) Successful in 12s
Details
check-linter.yaml / Checks with linters (pull_request) Successful in 28s
Details
check-syntax.yaml / Checks with static tools (pull_request) Successful in 23s
Details
check-yamllint.yaml / Lint yaml files (pull_request) Successful in 3s
Details
f3b49b53bd
davide requested review from llm-bot 2026-07-16 14:35:46 +00:00
llm-bot reviewed 2026-07-16 15:38:08 +00:00
llm-bot left a comment
Copy link

PR Review: feat: major ui refactor

This is a significant UI refactor that introduces a new sidebar-based layout, navigation indicators, and restructures several routes. Here are my findings:

✅ Positive Changes

  1. Well-structured sidebar system — The new SidebarRoot, SidebarContent, and SidebarButton components are clean and follow Svelte 5 patterns well. The mobile/desktop responsive behavior (using MediaQuery) is handled correctly, and the sidebar auto-closes on mobile navigation via afterNavigate.

  2. Navigation indicators — The Dimmer.svelte and ProgressBar.svelte components are thoughtful UX additions. They use $state and lifecycle hooks (beforeNavigate, afterNavigate) correctly. The randomized progress animation is a nice touch.

  3. Cart/Order improvements — Adding getItems() and getTotal() methods to the Cart class, plus refactoring cart item batch to include prices, is a solid improvement. Moving calculateProductOptionPrice from the client to the module-level function in option/index.ts is a good architectural change.

  4. FormOrConfigureDialog — The new dialog component for product ordering is well-designed, handling both simple products (no options) and configurable ones elegantly.

  5. Header component — Clean extraction of the header with user popover, sidebar toggle, and logout functionality.

⚠️ Issues to Address

1. Hardcoded placeholder in admin/+page.svelte

<Header />
hello
<!--
- productsLoop:
...
-->

The page contains literal hello text and a large TODO comment block. This needs to be replaced with actual admin dashboard content before merging.

2. Border color inconsistency

Several admin pages use border-slate-300 instead of the custom border-mist-default:

  • src/routes/admin/printers/+page.svelte — .list-column uses border-slate-300
  • src/routes/admin/printers/[printerId]/+page.svelte — .list-column uses border-slate-300

The custom palette defines border-mist-default as border-mist-200 dark:border-mist-800. These should use the consistent custom utility.

3. Deleted EmailInput component

EmailInput.svelte was removed. Verify that no routes or components still reference it (grep showed no remaining references, but double-checking is recommended).

4. Removed controls/index.ts re-exports

A.svelte and Button.svelte were deleted and removed from the index. The +error.svelte route was correctly updated to use a native <a> tag. Ensure all other usages of these components were migrated.

5. Favicon reorganization

Static files were moved from static/ to static/favicon/. Ensure the migration is complete and no old paths are referenced anywhere. Also verify that the new web-app-manifest-192x192.png and web-app-manifest-512x512.png files are the correct assets.

6. RadioInput defaultEntryUI is not customizable

The defaultEntryUI snippet in RadioInput.svelte is defined but not exposed as a prop. The entryUI prop allows customizing individual entries, but there's no way to customize the default rendering. This might be intentional, but worth noting.

7. Biome internal error

The lint:check CI step shows a Biome panic:

This is a bug in Biome, not an error in your code, and we would appreciate to report it
Message: index out of bounds: the len is 40 but the index is 1116

This occurs in the cups/ workspace and appears to be a Biome bug, not a code issue. However, it's worth investigating or skipping that workspace in CI.

ℹ️ Notes

  • No test files found — The test step exits with code 1 because no test files exist. This is a pre-existing condition, not introduced by this PR.
  • Build requires env vars — The build fails due to missing environment variables (expected in local dev). CI should have these configured.
  • Tailwind v4 @reference/@apply warnings — Svelte-check shows warnings for @reference and @apply rules. These are valid Tailwind v4 syntax and can be safely ignored or suppressed in the check config.

📊 Summary

Category Count
New components ~8
Deleted components 4
Modified routes ~25
Files changed 93
Lines added +1146
Lines removed -765

Overall: Solid refactor with good component organization. The main blocker is the placeholder hello text in admin/+page.svelte. The border color inconsistency should also be fixed for visual consistency.

# PR Review: feat: major ui refactor This is a significant UI refactor that introduces a new sidebar-based layout, navigation indicators, and restructures several routes. Here are my findings: ## ✅ Positive Changes 1. **Well-structured sidebar system** — The new `SidebarRoot`, `SidebarContent`, and `SidebarButton` components are clean and follow Svelte 5 patterns well. The mobile/desktop responsive behavior (using `MediaQuery`) is handled correctly, and the sidebar auto-closes on mobile navigation via `afterNavigate`. 2. **Navigation indicators** — The `Dimmer.svelte` and `ProgressBar.svelte` components are thoughtful UX additions. They use `$state` and lifecycle hooks (`beforeNavigate`, `afterNavigate`) correctly. The randomized progress animation is a nice touch. 3. **Cart/Order improvements** — Adding `getItems()` and `getTotal()` methods to the `Cart` class, plus refactoring cart item batch to include prices, is a solid improvement. Moving `calculateProductOptionPrice` from the client to the module-level function in `option/index.ts` is a good architectural change. 4. **FormOrConfigureDialog** — The new dialog component for product ordering is well-designed, handling both simple products (no options) and configurable ones elegantly. 5. **Header component** — Clean extraction of the header with user popover, sidebar toggle, and logout functionality. ## ⚠️ Issues to Address ### 1. **Hardcoded placeholder in `admin/+page.svelte`** ```svelte <Header /> hello <!-- - productsLoop: ... --> ``` The page contains literal `hello` text and a large TODO comment block. This needs to be replaced with actual admin dashboard content before merging. ### 2. **Border color inconsistency** Several admin pages use `border-slate-300` instead of the custom `border-mist-default`: - `src/routes/admin/printers/+page.svelte` — `.list-column` uses `border-slate-300` - `src/routes/admin/printers/[printerId]/+page.svelte` — `.list-column` uses `border-slate-300` The custom palette defines `border-mist-default` as `border-mist-200 dark:border-mist-800`. These should use the consistent custom utility. ### 3. **Deleted `EmailInput` component** `EmailInput.svelte` was removed. Verify that no routes or components still reference it (grep showed no remaining references, but double-checking is recommended). ### 4. **Removed `controls/index.ts` re-exports** `A.svelte` and `Button.svelte` were deleted and removed from the index. The `+error.svelte` route was correctly updated to use a native `<a>` tag. Ensure all other usages of these components were migrated. ### 5. **Favicon reorganization** Static files were moved from `static/` to `static/favicon/`. Ensure the migration is complete and no old paths are referenced anywhere. Also verify that the new `web-app-manifest-192x192.png` and `web-app-manifest-512x512.png` files are the correct assets. ### 6. **RadioInput defaultEntryUI is not customizable** The `defaultEntryUI` snippet in `RadioInput.svelte` is defined but not exposed as a prop. The `entryUI` prop allows customizing individual entries, but there's no way to customize the default rendering. This might be intentional, but worth noting. ### 7. **Biome internal error** The `lint:check` CI step shows a Biome panic: ``` This is a bug in Biome, not an error in your code, and we would appreciate to report it Message: index out of bounds: the len is 40 but the index is 1116 ``` This occurs in the `cups/` workspace and appears to be a Biome bug, not a code issue. However, it's worth investigating or skipping that workspace in CI. ## ℹ️ Notes - **No test files found** — The test step exits with code 1 because no test files exist. This is a pre-existing condition, not introduced by this PR. - **Build requires env vars** — The build fails due to missing environment variables (expected in local dev). CI should have these configured. - **Tailwind v4 `@reference`/`@apply` warnings** — Svelte-check shows warnings for `@reference` and `@apply` rules. These are valid Tailwind v4 syntax and can be safely ignored or suppressed in the check config. ## 📊 Summary | Category | Count | |----------|-------| | New components | ~8 | | Deleted components | 4 | | Modified routes | ~25 | | Files changed | 93 | | Lines added | +1146 | | Lines removed | -765 | **Overall**: Solid refactor with good component organization. The main blocker is the placeholder `hello` text in `admin/+page.svelte`. The border color inconsistency should also be fixed for visual consistency.
src/routes/admin/+page.svelte
@ -3,4 +3,4 @@
import { requireAdmin } from "#lib/auth/index.remote.ts";
await requireAdmin();
</script>
llm-bot commented 2026-07-16 15:38:08 +00:00
Member
Copy link

Hardcoded 'hello' text and TODO comment block in admin dashboard page. This needs real content before merging.

Hardcoded 'hello' text and TODO comment block in admin dashboard page. This needs real content before merging.
src/routes/admin/printers/+page.svelte
llm-bot commented 2026-07-16 15:38:08 +00:00
Member
Copy link

Uses 'border-slate-300' instead of the custom 'border-mist-default' utility. Should use the project's custom palette for consistency.

Uses 'border-slate-300' instead of the custom 'border-mist-default' utility. Should use the project's custom palette for consistency.
src/routes/admin/printers/[printerId]/+page.svelte
llm-bot commented 2026-07-16 15:38:08 +00:00
Member
Copy link

Uses 'border-slate-300' instead of the custom 'border-mist-default' utility. Should use the project's custom palette for consistency.

Uses 'border-slate-300' instead of the custom 'border-mist-default' utility. Should use the project's custom palette for consistency.
davide merged commit 6908c8067e into main 2026-07-16 15:44:34 +00:00
davide deleted branch feat/ui 2026-07-16 15:44:34 +00:00
davide referenced this pull request from a commit 2026-07-16 15:44:35 +00:00
feat: major ui refactor (#97)
Sign in to join this conversation.
Reviewers
No reviewers
llm-bot
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
projects/events-cash-register!97
No description provided.