Skip to content
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

Global stylesheets (during create-svelte depending on the chosen CSS preprocessor) #726

Merged
merged 5 commits into from
Mar 29, 2021
Merged

Global stylesheets (during create-svelte depending on the chosen CSS preprocessor) #726

merged 5 commits into from
Mar 29, 2021

Conversation

babichjacob
Copy link
Member

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs global.css #714
  • This message body should clearly illustrate what problems it solves. Rationale in global.css #714. Additionally, this solves a "style leaking" problem where the :root font family declarations would be active on other pages navigated to from index that would then disappear on refresh. (This is a known issue from Sapper days so I won't elaborate more on it).
  • Ideally, include a test that fails without this PR but passes with it. We don't have tests for create-svelte. I tested the changes locally for all 3 languages.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

@vercel
Copy link

vercel bot commented Mar 27, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sveltejs/kit/BozB7StMKLNbvcgpk7pooK73dxQd
✅ Preview: Canceled

[Deployment for 01c2a4e canceled]

@babichjacob babichjacob added the feature / enhancement New feature or request label Mar 27, 2021
@babichjacob
Copy link
Member Author

With the src/global.{css,less,scss} file path, the import in $layout.svelte is going to be relative: import '../global.{css,less,scss}';.

I'm not strongly attached to this path setup, so if we'd rather, for instance, do src/lib/global.{css,less,scss} instead so we can import '$lib/global.{css,less,scss}';--like if we think that encourages better practice--then I'd be good with that.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

I like that setup! I think it's well put within src, things within $lib feel more like reusable pieces to me.

.changeset/quiet-eels-greet.md Outdated Show resolved Hide resolved
@benmccann
Copy link
Member

benmccann commented Mar 27, 2021

So this is available on every page, but does not use svelte-preprocess's global rule, is that right? https://github.com/sveltejs/svelte-preprocess#global-style

I wonder if we should call it layout.css or app.css instead to avoid overloading the "global css" term?

@babichjacob
Copy link
Member Author

So this is available on every page, but does not use svelte-preprocess's global rule, is that right? https://github.com/sveltejs/svelte-preprocess#global-style

Right.

I wonder if we should call it layout.css instead to avoid overloading the "global css" term?

Here's a thought process:

  • src/layout.css and src/routes/$layout.svelte aren't next to each other.
  • If they were, there would be incongruity between $layout.svelte and layout.css (no $).
  • If it was renamed $layout.css, it'd be implied there was special meaning like all the other $s, but there is none.

I'm not particularly against the rename (without relocation) though.

@babichjacob
Copy link
Member Author

babichjacob commented Mar 27, 2021

Oh, I like src/app.css actually. Anyone else support renaming src/global.css (the current state of this PR) to that?

@babichjacob
Copy link
Member Author

Most recent commit switches to app.css and can be reverted if we decide global.css--or changed if we decide on another name--is better.

@GrygrFlzr
Copy link
Member

Kinda makes sense - the only thing that makes it "global" is being imported in src/routes/$layout.svelte. It's not really being treated as a special global file, so in a way this is a clearer name for it.

@rchrdnsh
Copy link

I like app.css as well :-)

@babichjacob
Copy link
Member Author

Kinda makes sense - the only thing that makes it "global" is being imported in src/routes/$layout.svelte. It's not really being treated as a special global file, so in a way this is a clearer name for it.

It's good that you bring that up because one hesitation I have with app.css is that it isn't inherently special like app.html is. You have to import it somewhere for it to do anything and you can get away with deleting it.

@dummdidumm
Copy link
Member

If we want it to be less "can infer meaning from the name" we could just call it styles.css, Angular does it for the globally attached style sheet, too.

@Rich-Harris
Copy link
Member

It's good that you bring that up because one hesitation I have with app.css is that it isn't inherently special like app.html is

It is slightly weird but not so much that I think it's a bad idea. I like this

@Rich-Harris Rich-Harris merged commit 7d42f72 into sveltejs:master Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants