Fix Sidebar Users Route Links #218
Conversation
Move default option values into the constructor parameter initializer to prevent them from being overwritten by the spread operator in `get options()`.
Replace literal theme union with AppTheme type in AdminController options and make path helpers resilient when admin_basepath is empty.
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review commnents
| "license": "FSL-1.1-MIT", | ||
| "dependencies": { | ||
| "@cfworker/json-schema": "^4.1.1", | ||
| "@clack/prompts": "^0.11.0", |
There was a problem hiding this comment.
I'm not sure why or when this dependency was removed, but in order to get the app running, I needed to bun add @clack/prompts
| --color-info: var(--color-blue-100); | ||
| --color-info-foreground: var(--color-blue-800); | ||
|
|
There was a problem hiding this comment.
from: bun run format
| "extends": "./tsconfig.json", | ||
| "compilerOptions": { | ||
| "outDir": "./dist/types", | ||
| "rootDir": "./src", | ||
| "baseUrl": ".", | ||
| "tsBuildInfoFile": "./dist/tsconfig.tsbuildinfo" | ||
| }, | ||
| "include": ["./src/**/*.ts", "./src/**/*.tsx"], | ||
| "exclude": ["./node_modules", "./__test__", "./e2e"] |
There was a problem hiding this comment.
from: bun run format
cameronapak
changed the title
|
@dswbx this is ready for review :) (No rush) |
|
@dswbx what would it take to get this reviewed? (P.S. Hope you're well, brother!) |
|
@cameronapak sorry I was so caught in bundling the release and prepping the mcp stuff. I try to review/merge this weekend, but monday the latest :) hope that‘s okay! |
…16-fix-users-link # Conflicts: # bun.lock
|
@cameronapak thanks a lot! It does indeed solve the case when admin was served statically at a different basepath. However, it fails when you use the Admin as React component, e.g. on the astro example: 
I'll try to take a look at it, but I'm a bit limited in time today/tomorrow. Could you also reduce the tests? Those are a bit too comprehensive for testing only the basepath handling. |
|
@cameronapak actually supplied a fix, could you check? you'd also need to test this in the astro example: |
…fork/cameronapak/cp/216-fix-users-link
The `admin_basepath` configuration option has been renamed to `basepath` for clarity and consistency. This change affects how the admin base path is accessed and utilized within the `AppReduced` utility. Tests have been updated to reflect this renaming.
The Response.blob() method in JavaScript returns a Blob, not a File. This means that file-specific metadata like `name` and `lastModified` is lost. The tests were updated to reflect this by manually constructing a File object from the Blob, mimicking how client code would handle this conversion. The MIME type is now correctly preserved from the response headers when creating the File.
|
@dswbx I can now confirm that this works when I run the astro app and test locally, as well as the tests pass. This is ready for re-review! P.S. I know you're on a time of rest, holiday, and family. May God bless you and the time be fulfilling. This PR can wait. |
|
This is ready to be tested, reviewed, and if approved pushed to the Having it where the admin dashboard fails to resolve the right admin pages when clicked breaks the admin dashboard experience and is therefore a very high priority |
2 participants
Fixes #216
Fixed a bug where the Users and Settings (icon) linked to pages in the admin but didn't include the
admin_basepath.