Conversation
deployed preview: https://203.connect-d5y.pages.devWelcome to connect! Make sure to:
Mobile
Desktop
|
greatgitsby
force-pushed
the
feat/upload
branch
3 times, most recently
from
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 |
greatgitsby
force-pushed
the
feat/upload
branch
from
b449ba8 to
2fd23c1
Compare
greatgitsby
force-pushed
the
feat/upload
branch
from
2fd23c1 to
db62cef
Compare
greatgitsby
changed the title
greatgitsby
force-pushed
the
feat/upload
branch
4 times, most recently
from
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) |
|
This branch is behind commaai/master. The line count diff bot is disabled. |
greatgitsby
force-pushed
the
feat/upload
branch
from
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 |
|
created these which are self-contained: commaai/connect#235 i'll do some component refactors later today |
greatgitsby
force-pushed
the
feat/upload
branch
from
cf2c6b3 to
2c83971
Compare
greatgitsby
force-pushed
the
feat/upload
branch
from
3b80103 to
4bd9300
Compare
| { | ||
| label: 'Uploading', | ||
| value: () => | ||
| queue() | ||
| ?.items() | ||
| .filter((i) => i.status === 'uploading').length, | ||
| }, | ||
| { | ||
| label: 'Waiting', | ||
| value: () => | ||
| queue() | ||
| ?.items() | ||
| .filter((i) => i.status === 'queued').length, | ||
| }, | ||
| { label: 'Total', value: () => queue()?.items().length }, |
There was a problem hiding this comment.
this formatting can be more compact but Biome was doing this.
| const sortedItems = createMemo(() => { | ||
| const allItems = [...items.offline, ...(offline() ? [] : items.online)] | ||
| return allItems.sort((a, b) => { | ||
| const statusDiff = getStatusPriority(a.status) - getStatusPriority(b.status) | ||
| if (statusDiff !== 0) return statusDiff | ||
| const routeDiff = a.route.localeCompare(b.route) | ||
| if (routeDiff !== 0) return routeDiff | ||
| const segmentDiff = a.segment - b.segment | ||
| if (segmentDiff !== 0) return segmentDiff | ||
| return a.filename.localeCompare(b.filename) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
i think i will setup a stable sort for the uploader queue on Athena. we might be able to let the device dictate the order (more than just priority) so that the animation is consistent
for now we can at least stable sort it visually
incognitojam
left a comment
incognitojam
left a comment
There was a problem hiding this comment.
The upload queue looks good and is polished with animations and everything, but I don't think I can review this all at once still. It's just not possible for me to look at how this all works and give good feedback or ask questions...
What does a minimum upload queue look like? Could you make a version of this under 200 lines? Then you can open as many PRs as you like afterwards to add things (if you think these can be split out, just some ideas):
- animations
- clearing the queue
- showing the offline queue
By the way, this is 9% of the lines in the app!
| @@ -0,0 +1,52 @@ | |||
| import { Component, createMemo } from 'solid-js' | |||
There was a problem hiding this comment.
I would just put all of the components in the same file if they aren't going to be used elsewhere in the app
| url: string | ||
| } | ||
|
|
||
| export interface UploadItem { |
There was a problem hiding this comment.
Can this live in an UploadQueue.tsx too?
| import QueueItem from './QueueItem' | ||
| import clsx from 'clsx' | ||
|
|
||
| const animations = (slide: boolean) => { |
There was a problem hiding this comment.
Animations look like something which could just be added in a separate PR later, where there is time to test them properly
|
hey, thanks for the review. sure, i can pair this down to start.. |
|
breaking these features into separate PRs won't change the lines of code it will introduce into the app. let's forgo them until there is a need, i guess. |
|
closing in favor of commaai/connect#251, a simplified version of this PR |
2 participants
second part of commaai/connect#51