-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: upload queue #203
base: master
Are you sure you want to change the base?
feat: upload queue #203
Conversation
deployed preview: https://203.connect-d5y.pages.devWelcome to connect! Make sure to:
Mobile
Desktop
|
ee8087a
to
c538949
Compare
how flexible are the LoC and bundle size numbers? i will do some refactoring but there will be limits. |
The hard limit is 500 kb, 5000 lines I see about 200 lines is in the API fetchers, another 100 for types - for now I think you can save a bunch of lines by wrapping the lines less. I'd like to move this out to a separate package later if it's all standard comma API stuff (there is already a comma API package but it doesn't have types) |
thanks! yea moving api calls and types out will help quite a lot. i'll do some refactors and it should be achievable |
Need any help finishing this? You could also split the requesting uploads and upload queue into separate PRs |
nope, i've been working on the clip bounty. i'm planning on separating the API contract and calls separately as that is well defined now |
b449ba8
to
2fd23c1
Compare
2fd23c1
to
db62cef
Compare
091b189
to
a3d05b4
Compare
Can you split this up at all? It's pretty difficult to review (e.g. small fixes can be their own PRs, StatisticsBar refactor..) |
let me see what i can do. some refactors were for DRY (like the stats bar, that code was already somewhere else) |
Changes:
|
3c56846
to
f3f6d5a
Compare
removed the stats bar component and changes to the progress bar |
I just meant you could open PRs to fix/cleanup things first, which would be small and easy to merge, if they help make the diff for this PR smaller |
@@ -52,6 +53,7 @@ | |||
"playwright": "^1.51.1", | |||
"postcss": "^8.5.3", | |||
"solid-devtools": "^0.33.0", | |||
"solid-transition-group": "^0.3.0", |
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.
this is a normal dep instead of a dev dep?
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.
sorry, used to putting everything in dev for FE apps. will move it since it's the standard here
@@ -40,6 +40,7 @@ | |||
}, | |||
"devDependencies": { | |||
"@biomejs/biome": "1.9.4", | |||
"@solid-primitives/timer": "^1.4.0", |
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.
is this used?
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.
nice catch, was using it but not anymore. removing in next rev
second part of #51