Skip to content

feat: improve minimal template #643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jul 26, 2025

Conversation

jycouet
Copy link
Contributor

@jycouet jycouet commented Jul 25, 2025

Closes: #218

  • Having a +layout.svelte (with the h1)
  • fav in /src/lib/assets (inline or immutable with assetsInlineLimit flag in vite.config)
  • Add a robots.txt to keep a static folder
  • Move playwright files beside routes like page.e2e.(j|t)s

Copy link

changeset-bot bot commented Jul 25, 2025

🦋 Changeset detected

Latest commit: 29b1dd6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
sv Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

pkg-pr-new bot commented Jul 25, 2025

Open in StackBlitz

npx https://pkg.pr.new/sveltejs/cli/sv@643
npx https://pkg.pr.new/sveltejs/cli/svelte-migrate@643

commit: 29b1dd6

@jycouet jycouet changed the title Skeleton template improvements Minimal template improvements Jul 25, 2025
@jycouet jycouet marked this pull request as draft July 25, 2025 21:47
@jycouet
Copy link
Contributor Author

jycouet commented Jul 25, 2025

Layout

I would still like to add a +layout.svelte with:

<script>
	import favicon from '$lib/assets/favicon.svg';

	let { children } = $props();
</script>

<svelte:head>
	<link rel="icon" href={favicon} />
</svelte:head>

<h1>Welcome to SvelteKit</h1>
{@render children?.()}

And update page.svelte.test.ts to layout.svelte.test.ts

Tests

Today when we have vitest & playwright, we have:

  • src/routes/page.svelte.test.ts
  • src/demo.spec.ts
  • e2e/demo.test.ts

So, some spec.ts, some test.ts, some in routes, some not. I think it's good to guide more here.

We could do:

  • src/routes/page.svelte.test.ts src/routes/page.svelte.spec.ts
  • src/demo.spec.ts
  • e2e/demo.test.ts

=> To have .spec.ts in all vitest files
=> To have .test.ts in all playwright files
=> e2e tests are not in the routes folder as they can describe a flow involving multiple pages.

Let me know your feelings :p

@jycouet jycouet marked this pull request as ready for review July 25, 2025 22:16
@manuel3108
Copy link
Member

I'm not totally sure I agree with moving the h1 in the layout, but I definitely agree for the favicon. So the layout file would still be present, just without and filled with the favicon. I think that's a good compromise.

I do agree with renaming that one vitest .test.ts file, it's totally inconsistent. Thanks for catching that. Also agree with your other conclusions!

@manuel3108
Copy link
Member

Ohh, and any reason you are only "linking" the issue in the pr description and not "closing" it?

@jycouet jycouet changed the title Minimal template improvements feat: improve minimal template Jul 26, 2025
@jycouet
Copy link
Contributor Author

jycouet commented Jul 26, 2025

I'm not totally sure I agree with moving the h1 in the layout, but I definitely agree for the favicon. So the layout file would still be present, just without and filled with the favicon. I think that's a good compromise.

I like this good compromise 😁

I do agree with renaming that one vitest .test.ts file, it's totally inconsistent. Thanks for catching that. Also agree with your other conclusions!

I updated only the test file name to match spec. Let's still keep the glob src/**/*.{test,spec}.{js,ts}. RIght ?

Ohh, and any reason you are only "linking" the issue in the pr description and not "closing" it?

Not really... It's just it was not addressing all points. Now that it's the case, I updated it ^^

@jycouet jycouet requested a review from manuel3108 July 26, 2025 06:30
@manuel3108
Copy link
Member

I updated only the test file name to match spec. Let's still keep the glob src/**/*.{test,spec}.{js,ts}. RIght ?

Yes, exactly.

Thinking out loud, any reason for not applying those changes to the other templates?

  • robots.txt is missing in library and has a different content in demo. I think it would make sense everywhere
  • favicon.svg. Moving into lib in library probably does not make sense, as it would probably be published as part of the library (although, maybe tree-shaking detects this?). Making the same changes we made here to demo makes sense I think.

Didn't make those changes myself, as I'm not 100% sure that's the correct way to do it. Let me know what you think!

@jycouet
Copy link
Contributor Author

jycouet commented Jul 26, 2025

  • robots.txt is missing in library

I think it's not really needed there. The first goal is to public a lib, not really deploying the website. No ? (Or maybe for the doc?!) Hummm, I don't know. I would not do it I think.

robots.txt has a different content in demo.

Yes, we could align with what we set here ?

  • favicon.svg. Moving into lib in library probably does not make sense, as it would probably be published as part of the library (although, maybe tree-shaking detects this?).

Yeah, let's not move favicon.svg in library.

favicon.svg Making the same changes we made here to demo makes sense I think.

Yeah, it's probably good to do this change

EDIT: Humm I'm not sure!
It already has import github from '$lib/images/github.svg'; showing immutability.
Also it's a folder images and not assets. What do you suggest ?


I'll update with your good thinking & my first opinions. Let's see after ;)

Copy link
Member

@manuel3108 manuel3108 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your last comment, let's leave it as is.

Thank you!

@manuel3108 manuel3108 merged commit 6abaa38 into sveltejs:main Jul 26, 2025
7 checks passed
@github-actions github-actions bot mentioned this pull request Jul 26, 2025
@hyunbinseo
Copy link
Contributor

The updated favicon code should be reverted. This issue is still open:

@jycouet
Copy link
Contributor Author

jycouet commented Jul 31, 2025

Oh wow, that's wild ! Didn't know about this :/

Is it better to revert the full PR or do another PR to bring back the old favicon only ? 👀

@hyunbinseo
Copy link
Contributor

If the favicon.svg stays in the static directory, is there a need to keep a generic robots.txt file?

In my personal template svelte-kitty, I am excluding certain predefined routes, making it meaningful.

User-agent: *
Disallow: /login
Disallow: /unsupported

https://github.com/hyunbinseo/svelte-kitty/blob/main/packages/create-svelte-kitty/template/static/robots.txt

@jycouet
Copy link
Contributor Author

jycouet commented Jul 31, 2025

If favicon.svg is going back, yes, there is no reason to keep this "empty" robots.txt.
But favicon.svg is temporary going back, right ? So, not sure about the robots.txt.

(The original idea was to have something in this folder so that people know that it exist)

Let me know, and I'll do.

Nice kitty!

@hyunbinseo
Copy link
Contributor

Yes, /static/favicon.svg is temporary.

It should be reverted to Vite inline asset import in the long term.

(The original idea was to have something in this folder so that people know that it exist)

This was my whole point with svelte-kitty and its structure. Then I stepped on the <svelte:head> issue.

@jycouet
Copy link
Contributor Author

jycouet commented Jul 31, 2025

#656

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skeleton template improvements
4 participants