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

Support CSS options in Vite projects #2245

Merged
merged 19 commits into from
Jul 8, 2024
Merged

Support CSS options in Vite projects #2245

merged 19 commits into from
Jul 8, 2024

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Jun 14, 2024

Needs #2240 merged first.

  • Add back --styling flag for the init command.
  • Support Vite projects in h2 setup css.
  • Deprecate CSS support for classic projects.
  • Use Tailwind v4 alpha for tailwind support.
image image

🎩 :

  • Try h2 setup css in the skeleton project.
  • Changes in h2 init might not be showing up due to an issue with the bundled CLI. I'm debugging this atm.

@frandiox frandiox requested a review from a team June 14, 2024 04:20
Copy link
Contributor

shopify bot commented Jun 14, 2024

Oxygen deployed a preview of your fd-styling-option branch. Details:

Storefront Status Preview link Deployment details Last update (UTC)
third-party-queries-caching ✅ Successful (Logs) Preview deployment Inspect deployment July 8, 2024 5:14 AM
optimistic-cart-ui ✅ Successful (Logs) Preview deployment Inspect deployment July 8, 2024 5:13 AM
custom-cart-method ✅ Successful (Logs) Preview deployment Inspect deployment July 8, 2024 5:14 AM
Skeleton (skeleton.hydrogen.shop) ✅ Successful (Logs) Preview deployment Inspect deployment July 8, 2024 5:13 AM
metaobjects ✅ Successful (Logs) Preview deployment Inspect deployment July 8, 2024 5:13 AM
classic-remix ✅ Successful (Logs) Preview deployment Inspect deployment July 8, 2024 5:13 AM

Learn more about Hydrogen's GitHub integration.

Copy link
Contributor

@blittle blittle left a comment

Choose a reason for hiding this comment

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

Just a few minor questions on the code. While testing it though, I run h2 setup css in the skeleton template, and I get this error when trying to start the project with npm run dev:

image

Not sure if the node_modules got messed up in the mono repo with the npm install, so I cd into the root and run npm install again. Then trying to start the dev server in the starter template, it appears to work, but I get these new errors:

image

Comment on lines 14 to 42
"@remix-run/react": "^2.9.2",
"@shopify/cli": "3.61.2",
"@shopify/cli-hydrogen": "^8.1.0",
"@shopify/hydrogen": "2024.4.3",
"@shopify/remix-oxygen": "^2.0.4",
"graphql": "^16.6.0",
"graphql-tag": "^2.12.6",
"isbot": "^3.6.6",
"react": "^18.2.0",
"react-dom": "^18.2.0"
},
"devDependencies": {
"@remix-run/dev": "^2.9.2",
"@remix-run/eslint-config": "^2.9.2",
"@shopify/mini-oxygen": "^3.0.3",
"@shopify/oxygen-workers-types": "^4.0.0",
"@shopify/prettier-config": "^1.1.2",
"@total-typescript/ts-reset": "^0.4.2",
"@types/eslint": "^8.4.10",
"@types/react": "^18.2.22",
"@types/react-dom": "^18.2.7",
"eslint": "^8.20.0",
"eslint-plugin-hydrogen": "0.12.2",
"postcss": "^8.4.21",
"postcss-import": "^15.1.0",
"postcss-preset-env": "^8.2.0",
"prettier": "^2.8.4",
"typescript": "^5.2.2"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it intentional to add all of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because the branch is not updated. I had to remove tailwind@v3 from this example but it has now been turned into a diff, so we can remove this part.

Comment on lines +3 to +6
"@vanilla-extract/css": "^1.15.2"
},
"devDependencies": {
"@vanilla-extract/css": "^1.11.0"
"@vanilla-extract/vite-plugin": "^4.0.10"
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that @vanilla-extra/css went from being a devDependency to a direct dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's what it's shown in the docs: https://vanilla-extract.style/documentation/getting-started/

I guess it doesn't really matter since this is just an app that is not published to NPM.

@@ -109,6 +125,7 @@ export async function runInit(
options.path ??= './hydrogen-quickstart';
options.routes ??= true;
options.shortcut ??= true;
options.styling ??= 'tailwind';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, tailwind default for quickstart.

Comment on lines +184 to +185
// doesn't match, but `export default {plugins:[]}` does.
// And `export default defineConfig({plugins:[]})` matches too.
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting how it supports both usecases.

@wizardlyhel
Copy link
Contributor

wizardlyhel commented Jul 3, 2024

I tested Vanilla Extract, CSS modules, PostCSS (using postcss-scss and postcss-plugin). These works fine.

Only tailwind v4 alpha, doesn't work. Inside skeleton template

  1. When running the command h2 setup css, it sets up the tailwind required changes.
  2. At this point, if you try to run npm run dev, you'll get that darwin-64 error that @blittle show. Not sure if it a monorepo thing but after running npm i at root folder, this error goes away
  3. Once you fix that darwin error and try to run npm run dev again, you get the postcss-import error that @blittle have

@frandiox
Copy link
Contributor Author

frandiox commented Jul 4, 2024

The first issue is likely related to this bug in NPM: npm/cli#4828
I'll try to find some workaround.

The second issue (postcss related), I think it's related to have tailwind v3 and v4 both installed locally. Will try to update this PR to ensure it's only Tailwind v4 there.

@frandiox
Copy link
Contributor Author

frandiox commented Jul 5, 2024

The second issue (postcss related), I think it's related to have tailwind v3 and v4 both installed locally. Will try to update this PR to ensure it's only Tailwind v4 there.

This one is now fixed after solving deps issues.

The first issue is likely related to this bug in NPM: npm/cli#4828

Unfortunately, this one seems trickier. Not sure if we can make it work in the monorepo 🤔

@frandiox
Copy link
Contributor Author

frandiox commented Jul 5, 2024

/snapit

@frandiox
Copy link
Contributor Author

frandiox commented Jul 8, 2024

The NPM bug also affects standalone projects and it doesn't seem to be possible to fix from within the CLI without creating bigger issues (e.g. removing package-lock).

This only affects the h2 setup css command, though, not h2 init. Therefore, for now at least, I'm just adding a warning:

image

Running npm install again fixes the issue.

@frandiox frandiox requested a review from a team July 8, 2024 05:12
@frandiox frandiox merged commit c269065 into main Jul 8, 2024
13 checks passed
@frandiox frandiox deleted the fd-styling-option branch July 8, 2024 08:53
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.

4 participants