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

Svelte widget #299

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Svelte widget #299

wants to merge 3 commits into from

Conversation

marySalvi
Copy link
Contributor

No description provided.

@marySalvi marySalvi requested a review from brianhelba August 25, 2023 18:18
@marySalvi marySalvi changed the base branch from master to switch-to-yarn August 25, 2023 18:19
@marySalvi marySalvi changed the title Svelte widget [WIP]Svelte widget Aug 25, 2023
@marySalvi marySalvi changed the base branch from switch-to-yarn to master December 5, 2023 15:55
@marySalvi marySalvi marked this pull request as ready for review December 5, 2023 16:04
@marySalvi marySalvi changed the title [WIP]Svelte widget Svelte widget Dec 5, 2023
@@ -27,7 +29,11 @@
"npm-run-all": "^4.1.5",
"parcel": "^2.10.0",
"rimraf": "^5.0.5",
"typescript": "^5.2.2"
"typescript": "^5.2.2",
"@tsconfig/svelte": "^5.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove @tsconfig/recommended as a dependency, since it's no longer used.

"django-s3-file-field": "file:../javascript-client"
"django-s3-file-field": "file:../javascript-client",
"svelte": "^4.2.0",
"svelte-preprocess": "^5.0.4"
},
"devDependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sort the devDependencies list.

"typescript": "^5.2.2",
"@tsconfig/svelte": "^5.0.2",
"parcel-transformer-svelte3-plus": "^0.2.10",
"process": "^0.11.10",
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove process, unless it's somehow needed.

@@ -0,0 +1,8 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

(This file is JSON5](https://parceljs.org/features/plugins/#.parcelrc), so let's use the same 2-space indent as other files.

Go ahead and add this file name to .editorconfig too.

@@ -0,0 +1,5 @@
const sveltePreprocess = require('svelte-preprocess')
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this file may not be necessary at all: https://github.com/HellButcher/parcel-transformer-svelte3-plus#configuration

If we do need it, I'd rather use .svelterc as a declarative JSON format, instead of svelte.config.js, if possible.

Comment on lines +5 to +7
"src/**/*.ts",
"src/**/*.js",
"src/**/*.svelte"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these necessary? Aren't they matched by *?

Comment on lines +49 to +51
"env": {
"node": true
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? We're compiling something for the browser, not Node.

console.log(arguments);
</script>

<h1>Hello {name}!</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

parcel-transformer-svelte3-plus is going to automatically enable svelte-preprocess for us (if it's installed), so we can use the template tag feature: https://github.com/sveltejs/svelte-preprocess#template-tag

So, I'd suggest wrapping the HTML in .svelte in <template> tags.

@@ -0,0 +1,25 @@
declare module '*.svelte' {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is any of this file necessary? Did you follow a tutorial that suggested creating this, or was it necessary to create by hand?

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

Successfully merging this pull request may close these issues.

2 participants