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

Implement infinite scroll in dashboard #501

Merged
merged 11 commits into from
Jun 18, 2024

Conversation

yaodingyd
Copy link
Contributor

Fix #493

2024-06-07_12-39-09.mp4

The console shows we keep fetching data until found; and it will fetch new data when user scrolls to the bottom.

There are some question remainng to be answered:

  1. I use a heuristic to do look back with some fallback. we might want to change that
  2. the option for time filer is not fixed yet. what should it be called? "Recent Rides"?
  3. I changed how the loading state display: when we have data and there's new data fetching, keep displaying current data and have a loader spiiner at the bottom.

Copy link

github-actions bot commented Jun 7, 2024

Welcome to connect! Make sure to:

  • read the contributing guidelines
  • mark your PR as a draft until it's ready to review
  • post the preview on Discord; feedback from users will speedup the PR review

deployed preview: https://501.connect-d5y.pages.dev

Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

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

Nice work!

  1. The routes_segments endpoint accepts a limit parameter. Something like this should work: set start to the end of loaded routes, end to start + 5 years, and limit to 10.
  2. I think we want to get rid of the time filter on top, and make specifying the time range a more niche thing (small button next to "take snapshot?"). It's very rare that you'll want to specify a range.
  3. Got a video of this?

.github/workflows/build.yaml Outdated Show resolved Hide resolved
.github/workflows/preview.yaml Outdated Show resolved Hide resolved
@yaodingyd
Copy link
Contributor Author

thanks for the review, will update per comment.

I've attached a short video in description, what information do you need more? I'll make a new one

@yaodingyd
Copy link
Contributor Author

The routes_segments endpoint accepts a limit parameter. Something like this should work: set start to the end of loaded routes, end to start + 5 years, and limit to 10.

I'm not sure if that would work. route_segments doesn't have this limit param:
https://github.com/commaai/comma-api/blob/6658a0e71e29ba6cfdb380f8b058ff91cd834719/src/drives.js#L13-L15

I see listRoutes has it
https://github.com/commaai/comma-api/blob/6658a0e71e29ba6cfdb380f8b058ff91cd834719/src/drives.js#L45-L51

but it doesn't return what i need

@yaodingyd
Copy link
Contributor Author

I replaced the time select dropdown with a button you suggested:

2024-06-07_23-21-27.mp4

@adeebshihadeh
Copy link
Contributor

I'm not sure if that would work. route_segments doesn't have this limit param: commaai/comma-api@6658a0e/src/drives.js#L13-L15

Ah that's incomplete. I was looking at the backend server code. If you add limit, it should work.

@yaodingyd
Copy link
Contributor Author

The api module is a npm package. We would need to update it first though

@adeebshihadeh
Copy link
Contributor

#503

@yaodingyd
Copy link
Contributor Author

I've updated the implementation for infinite scrolling, it would use routes_segment with incremental limit:

2024-06-08_18-38-38.mp4

I only use 3 as incremental value since there are not that many routes for demo account, and we can adjust this value

The manual time select still works

2024-06-08_18-39-37.mp4

@adeebshihadeh
Copy link
Contributor

I think 5 is a good increment. As a side effect, this should be a nice speedup for users who drive a lot!

Deployed preview doesn't work, but it still looks a bit buggy in your video. Let me know when it's ready for a proper review.

@yaodingyd
Copy link
Contributor Author

I think it's good to review, preview is good to test

2024-06-08_20-47-25.mp4

Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

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

It's still quite broken if you just scroll up and down repeatedly. Maybe try setting the increment to 1 to emulate on the demo route?

image

src/actions/index.js Show resolved Hide resolved
src/components/explorer.jsx Show resolved Hide resolved
src/components/DeviceInfo/index.jsx Outdated Show resolved Hide resolved
@yaodingyd
Copy link
Contributor Author

mind taking another look?

@adeebshihadeh
Copy link
Contributor

Still very broken. Immediately hit this on the demo:
image

@yaodingyd
Copy link
Contributor Author

I fixed another bug related to time select component but I'm having a hard time to reproduce the bug in your screenshot. I tested the latest preview with different browsers too:

chrome

2024-06-09_22-25-10.mp4

safari

2024-06-09_22-27-12.mp4

All look good, both for just scrolling and using time select.

Could it be a blip on your machine? Are you able to consistently reproduce the bug, and if so, could you give more details about your machine and browser?

@sshane
Copy link
Contributor

sshane commented Jun 10, 2024

I would add a little more padding on the bottom of "There are no more routes found in selected time range."

@yaodingyd
Copy link
Contributor Author

@adeebshihadeh sorry for the ping, what's the next step here?

@adeebshihadeh
Copy link
Contributor

Not sure if you fixed it, but I couldn't repro the bug when I went to screen record it. Can you rebase on the latest master?

@adeebshihadeh
Copy link
Contributor

Instantly hit it again on Chrome on my Ubuntu machine. Here's a screen recording:

infinite_scroll_bug.mp4

@yaodingyd
Copy link
Contributor Author

Finally figured out the issue: there is a race condition that the next page fetch would void the previous fetching in flight, like events and locations. I changed the code that we store any fetched event/location, and update state would always try to update all these stored event/location. Could you give it another try?

@adeebshihadeh adeebshihadeh merged commit 6129905 into commaai:master Jun 18, 2024
4 checks passed
@yaodingyd yaodingyd deleted the inifite-scroll branch June 19, 2024 00:07
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.

Replace default time window with last N drives
3 participants