[cli] Clean templates, fix a bunch of issues in generated apps #4040
Conversation
bharatkashyap
added
the
create-toolpad-app
bharatkashyap
requested review from
a team and
Janpot
and removed request for
a team
bharatkashyap
requested review from
a team and
apedroferreira
and removed request for
a team
| @@ -40,7 +39,7 @@ const NAVIGATION: Navigation = [ | |||
| ]; | |||
|
|
|||
| const BRANDING = { | |||
| title: 'My Toolpad Core App', | |||
| title: 'My Toolpad Core Next.js Pages App', | |||
There was a problem hiding this comment.
It makes the app easier to identify while testing/running - in all likelihood a person running this example is going to change these anyway regardless of whatever the default text is
There was a problem hiding this comment.
It makes the app easier to identify while testing/running
Sure, but that's not a problem our users would have, they know what they picked. We can't do it for all permutations, so I propose we do it for none.
e.g. why not
title: 'My Toolpad Core Next.js Pages App with Google+Github auth and joy theme',🙂
There was a problem hiding this comment.
I propose we only do it for things that aren't immediately visible on the app (Auth providers/Theme/Branding are all visible and obvious). Currently it would only be the type of router; if we introduce something that fits this criteria in the future, we could do it for that as well
There was a problem hiding this comment.
The previous title was already looking a bit too long for the current layout, I would also prefer to use shorter titles...
There was a problem hiding this comment.
Removed the longer titles
| if (missingVars.length > 0) { | ||
| const baseMessage = 'Authentication is configured but the following environment variables are missing:'; | ||
|
|
||
| if (process.env.NODE_ENV === 'production') { |
There was a problem hiding this comment.
Avoid different behavior in dev vs. prod. If it crashes in prod it must crash in dev. Let's just warn then.
Suggestion:
function warnOnMissing(var: string, provider: string, name: string) {
if (!var) {
console.warn(`${provider} provider: environment variable "${name}" missing`);
}
}
warnOnMissing(process.env.GOOGLE_CLIENT_ID, 'Google', 'GOOGLE_CLIENT_ID')
warnOnMissing(process.env.GOOGLE_CLIENT_SECRET, 'Google', 'GOOGLE_CLIENT_SECRET')
// ...(Avoid dynamically accessing process.env[name], some runtimes want to statically analyze)
| @@ -0,0 +1,46 @@ | |||
| import { SupportedRouter } from '../types'; | |||
|
|
|||
| const indexPage = (authEnabled: boolean, routerType: SupportedRouter) => { | |||
There was a problem hiding this comment.
Can't we just give all templates the same options as an object? That way we only have to create that once and can pass it to each template.
| @@ -27,6 +27,7 @@ const NAVIGATION: Navigation = [ | |||
| title: 'Main items', | |||
| }, | |||
| { | |||
| segment: '', | |||
There was a problem hiding this comment.
The default segment is already '' so we don't need to include it.
Fixes [create-toolpad-app] Issues with pages router app #4005
Fixes create-toolpad-app: align created apps content #3973
Fixes create-toolpad-app: add warning on missing configuration #3972
Also cleans up the template files to have one file per "file type" to avoid duplication
Also brings the playground apps in line with the generated templates
Also brings the example apps in line with the generated templates
Also makes a small change to the
SignInPageto better support longer app names