-
Notifications
You must be signed in to change notification settings - Fork 58
Upgrade to svelte 5 and bump all other dependencies #101
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
base: svelte5
Are you sure you want to change the base?
Conversation
|
Wow, this PR is huge. I need some time to review it 😂 |
3 and 4. I think I know what this is - will double check this as well and get back to you. Thanks for the quick reply! After this I'll try to give the UI some (conservative) love. Any ideas yourself or preferences? |
Main goal is not make UI mobile-oriented 🙂 |
|
What about eslint and prettier - I prefer to make them optional (project README has line about that), but having configs for them. |
|
Hmm, so thats a bit awkward. Generally javascript projects have them on or off, both for the ease of contributing but also to standardize code style. Because if you allow it to be off sometimes then folks can and will just submit code in any style. That kind of defeats the purpose of having them all together. There are no post-commit hooks or anything like that that forces folks to use it in this PR - but it will set it up for (arguably the sane 😂) folks if they want. Also the way it was currently setup would require someone to have prettier and eslint installed globally which is a big no-no Regarding UI stuff - perfect, that's what I was thinking as well. Just some simple tweaks to make it more comfortable to use on the desktop first and foremost. How attached areyou to the capacitor apk build? What do you think about shipping the web app as a progressive web app (PWA) so folks can "install" the web app on mobile devices? |
Personally I am using IDE prettier extension and prettier config in project dir is enough.
README entry is meant for installing eslint in the project directory, not globally. Anyway, I am not very experienced web dev and if you sure that I need to include them as dependency, it is ok.
It was done already if I understand correctly: niimblue.webmanifest. |
|
Yeah so installing the prettier extension in effect installs it "globally" for you / the projects you open in VSCode. But, for example, anyone using another editor (i use vim for example) would want it to be installed "normally". I'd highly suggest going this route with eslint/prettier 👍 I was just curious about the capacitor stuff, i will make sure it stays then! About PWA, you need more than just a manifest file afaik. I definitely couldn't install it on Vivaldi android (chrome 140) 😅 It's also missing a specific png img size apparently (https://www.pwabuilder.com/reportcard?site=https://niim.blue), but no worries. Not super important right now. I'll remove Sveltekit and clean this up a bit further this weekend and we can go from there ❤️ |
Fixed |
|
Okay, I've removed SvelteKit and addressed some of your feedback. To get to your last msg specifics:
Don't let JS hate on the internet get to you btw 😅, I see you're active in many non-web dev areas of software development. Of course keeping dependencies low generally is not a bad goal, but almost none of these would have gotten shipped to the client. We had basically only added I'd recommend taking a look at something like Anthony Fu's For client-side bundle size, I like They both then open a nice UI in the browser locally to inspect things. Not sure exactly what you mean here. When I load it during dev (and clear all cache / localstorage / etc.), I get NotoSans by default and all other font things seem to work. Can you help me understand what the issue is here?
Can you help me reproduce this? Maybe was an issue only when we had sveltekit on. It seems to work as expected now
SvelteKit would add those in the build process via stuff like this (see screenshot), you shouldn't put them in the root
|
Still broken. Press save multiple times. Do not change the name. Your branch: vivaldi_hrHmkVHsiZ.mp4Main branch: vivaldi_zODTANFapH.mp4 |
It fixed by reverting font preload meta tags (below |
|
Okay I've removed those font preload tags and updated the package-lock, forgot to commit that after sveltekit removal. Regarding the label behavior. It's because the key for that Unfortunately there isn't any globally unique field on labels like an |
|
Ahh i thought you were telling me you tried it and I should remove the font meta tags 😂 You can't use "class" as prop because that's a keyword in js (i.e. for actual classes, so had to rename to className in the destructuring) Time stamp isn't great as key, as you mentioned, but maybe best for now Okay, let's get this finished.. Will check the remaining comments and test that everything works as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- index.html still not reverted
- locale files still not reverted / excluded from formatting
- project uses npm, not pnpm, remove pnpm-lock.
- svelte.config.js: type comment should be
/** @type {import("@sveltejs/vite-plugin-svelte").SvelteConfig} */ - there are lot of eslint errors about
#eachkeys - I think we need to disable build warnings about non-labeled close buttons:
const config = {
preprocess: vitePreprocess(),
compilerOptions: {
warningFilter: (warning) => {
return warning.code !== 'a11y_consider_explicit_label';
}
}
};- You need to merge upstream changes.
Still, it wasn't the best idea to combine migration, refactoring, and formatting into a single pull request 😔
|
Yeah we'll get through them, I've fixed most of the above. The lint issues are stuff you should probably address tbh. Like the each having key ones - otherwise handling objects in each blocks can easily get the dom confused. Ofc feel free to add the ignore of the button label warning, although that is there for a reason too. That one is just a warning though, right? |
|
I fixed all the remaining eslint issues (including the each keys) in that last commit. But give it a test please. I'm not sure where they're all used. Hopefully there's not much more remaining 😅. Appreciate you working with me on this! |
| let printState: "idle" | "sending" | "printing" = $state("idle"); | ||
| let modal: Modal; | ||
| let printProgress: number = 0; // todo: more progress data | ||
| let density: number = $printerMeta?.densityDefault ?? 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems lile some state fields not converted to new state syntax (density/quantity/postProcessType and other).
|
Interesting, i used their auto migrate tool which i would have hoped caught all of the But no worries, I'll go back and get those 👌 |
|
It was broken on upstream merge commit (6b7d48e). |
|
Fixed the on:clicks and it builds again now |










Hey man, was sooo happy to have found this when I got my niim labelmaker. As a web dev I want to help improve the UI, but first I figured this could use a nice little pass to ensure all eslint/prettier stuff is setup correctly and that all dependencies are up to date. Hope I haven't changed too much on you 😅
This PR does the following: