Skip to content

Conversation

d3lm
Copy link
Contributor

@d3lm d3lm commented Jun 24, 2024

This PR adds our eslint plugin to enforce our coding convention across the monorepo.

I have already fixed all linting issues that were reported and streamlined some of the CSS files as well to match our conventions. We should definitely add some style (CSS, SCSS) linting as well at some point.

ℹ️ Note that running this won't work as of right now because the new version of the eslint plugin is not yet published.

Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented Jun 24, 2024

⚠️ No Changeset found

Latest commit: 78543e4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@d3lm d3lm requested review from SamVerschueren and Nemikolh June 24, 2024 14:06
@Nemikolh Nemikolh requested a review from sulco June 24, 2024 14:40
@d3lm
Copy link
Contributor Author

d3lm commented Jun 24, 2024

Updated the PR to use the alpha version that was released.

package.json Outdated
@@ -11,11 +11,13 @@
"docs:build": "pnpm run --filter=tutorialkit.dev build",
"demo": "pnpm run --filter=demo.tutorialkit.dev dev",
"demo:build": "pnpm run --filter=demo.tutorialkit.dev build",
"lint": "eslint \"{packages,docs}/**/*.{ts,tsx,js,json,jsonc,js,mjs,cjs}\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is eslint covering prettier as well?

Any reason we don't include all files and folders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, our eslint plugin always included the eslint recommended rules and it also takes the prettier config into account.

Any reason we don't include all files and folders?

I only did this to reduce the number of files to be checked and to be more specific. Other than that there's no particular reason. Cause those extensions are the only files that our plugin covers, but I think it would be fine to remove the extensions since the plugin contains the file extensions too and it would ignore other files.

Copy link
Contributor

@SamVerschueren SamVerschueren left a comment

Choose a reason for hiding this comment

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

Great work @d3lm 🙏 . Love to see the consistency!

Copy link
Member

@Nemikolh Nemikolh left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

import { TutorialStore } from '@tutorialkit/runtime';
import { auth, WebContainer } from '@webcontainer/api';
import { useAuth } from './setup.js';
Copy link
Member

Choose a reason for hiding this comment

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

Oh this import should always be first as it sets a global read by the @webcontainer/api package.

Copy link

cloudflare-workers-and-pages bot commented Jun 25, 2024

Deploying tutorialkit-demo-page with  Cloudflare Pages  Cloudflare Pages

Latest commit: 35e23d8
Status:🚫  Build failed.

View logs

@d3lm d3lm requested review from Nemikolh and SamVerschueren June 25, 2024 12:24
Copy link
Member

@Nemikolh Nemikolh left a comment

Choose a reason for hiding this comment

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

This is amazing!! Really good work both here and on the eslint plugin @d3lm

🎉 🎉

@Nemikolh Nemikolh merged commit fcfb3e8 into main Jun 25, 2024
@Nemikolh Nemikolh deleted the delm/feat/eslint branch June 25, 2024 12:32
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.

3 participants