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

Replace raw files popup with a table #277 #487

Closed
wants to merge 26 commits into from

Conversation

RossGGG
Copy link

@RossGGG RossGGG commented Jun 2, 2024

This pull request fixes #277 .

  • Added material-ui x DataGrid and styling dependencies to package.json
  • New Menu item in 'Files' popup to view 'Route Files'
  • New FilesTable modal that utilizes mui DataGrid component to display a table with links for all segment(s) uploaded files.

RossGGG added 2 commits June 2, 2024 16:20
* Added material-ui x DataGrid and styling dependencies to package.json
* New Menu item in 'Files' popup to view 'Route Files'
* New FilesTable modal that utilizes mui DataGrid component to display a table with links for all segment(s) uploaded files.
@RossGGG RossGGG marked this pull request as ready for review June 2, 2024 23:42
@adeebshihadeh
Copy link
Contributor

Looks like this isn't exposed in the UI yet?

@macdoos
Copy link

macdoos commented Jun 3, 2024

Sup, I've been following this PR and managed to get the table showing in the modal with a couple of changes to the FileTable.jsx. Would you like me to push a commit?

Screenshot 2024-06-03 at 08 05 59

@RossGGG
Copy link
Author

RossGGG commented Jun 3, 2024

Looks like this isn't exposed in the UI yet?

Sorry about that. It looks like those changes got wiped when I did a rebase. I re-committed the missing changes to add the menu item.
@macdoos I might have pushed similar changes just now. Let me know.

image

@RossGGG
Copy link
Author

RossGGG commented Jun 3, 2024

And here is the table opened up with the demo data.
image

@adeebshihadeh
Copy link
Contributor

Looks pretty good, but didn't work for me. Here's the console log.
localhost-1717449008310.log

@RossGGG
Copy link
Author

RossGGG commented Jun 3, 2024

Looks pretty good, but didn't work for me. Here's the console log. localhost-1717449008310.log

Try rebuilding your node_modules repository with a fresh pnpm install. I had to add the dependencies for x-data-grid to package.json and that extra step was necessary for me to get it to load. If that's not it, were you trying it with the demo data? I don't have any comma hardware, so perhaps there a case that needs to be handled with the live data...

@macdoos
Copy link

macdoos commented Jun 4, 2024

@RossGGG I don't have permission to push to this PR. Regarding the data, if you want to populate the table, you can use the app in demo mode.

@RossGGG
Copy link
Author

RossGGG commented Jun 4, 2024

@macdoos Yeah I did that, and it works fine with the demo data. Not sure why the test suite is showing failed now. It works locally.

@macdoos
Copy link

macdoos commented Jun 5, 2024

@RossGGG Have you tried running the tests locally? Also, could you please give me write access to this PR so I can try?

@RossGGG
Copy link
Author

RossGGG commented Jun 5, 2024

@RossGGG Have you tried running the tests locally? Also, could you please give me write access to this PR so I can try?

I ran the tests locally on the live repo and they went fine every time. When I do a build and run the tests, it gets more inconsistent but usually works. When I serve the build and go through the steps manually they always produce the expected results, and the error messages the automated tests give when they have failed have been too generic to point to any underlying issue.

I think you'll need access to my fork to update the PR. I just sent you a collaboration invite.

@macdoos macdoos force-pushed the raw_files_table branch from dbb373f to ea20c5a Compare June 5, 2024 04:18
@RossGGG
Copy link
Author

RossGGG commented Jun 5, 2024

@macdoos Seems to work for me. It was working before your latest commit too, but good to know it works for multiple people now.

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.

There's a bug where the second click on the "Files" button opens the new table instead of the intermediate popup.

Comment on lines +32 to +34
"@mui/x-data-grid": "^7.6.1",
"@emotion/styled": "^11.11.5",
"@emotion/react": "^11.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

what are these for? a table shouldn't really require any new dependencies

Copy link
Author

Choose a reason for hiding this comment

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

I used the x-data-grid react component because the Table react component in the legacy version of material-ui being used for this project does not support virtualized paging and I assumed live datasets could contain a larger number of segments and files than the demo dataset provided. If this assumption was incorrect, I can update it to use the Table react component instead.

src/__puppeteer__/drive.test.js Outdated Show resolved Hide resolved
This reverts commit ea20c5a.

Revert "Increasing drive.test timeout to 80000ms"

This reverts commit 5cf4d9d.

Revert "Increase drive.test timeout to 100000ms"

This reverts commit 3b47eaa.
@RossGGG
Copy link
Author

RossGGG commented Jun 6, 2024

There's a bug where the second click on the "Files" button opens the new table instead of the intermediate popup.
@adeebshihadeh
I have reverted the commit that introduced this bug. It should behave as expected again.

@adeebshihadeh
Copy link
Contributor

Can you rebase to get the new preview deployment? Then you can share the link on our Discord and have some people stress test it.

@RossGGG
Copy link
Author

RossGGG commented Jun 6, 2024

Can you rebase to get the new preview deployment? Then you can share the link on our Discord and have some people stress test it.

Yeah that sounds good. I will go do that now.

RossGGG added 3 commits June 5, 2024 19:52
* Added material-ui x DataGrid and styling dependencies to package.json
* New Menu item in 'Files' popup to view 'Route Files'
* New FilesTable modal that utilizes mui DataGrid component to display a table with links for all segment(s) uploaded files.
RossGGG and others added 9 commits June 5, 2024 19:52
This reverts commit ea20c5a.

Revert "Increasing drive.test timeout to 80000ms"

This reverts commit 5cf4d9d.

Revert "Increase drive.test timeout to 100000ms"

This reverts commit 3b47eaa.
* Fix table for segments that do not have available uploads for certain file types.
* Fix style for header filler cells if columns do not span the full width of the table.
…table

# Conflicts:
#	src/components/Files/FileTable.jsx
@RossGGG
Copy link
Author

RossGGG commented Jun 6, 2024

Can you rebase to get the new preview deployment? Then you can share the link on our Discord and have some people stress test it.

The preview deployment is failing due to a missing secret (apiKey) for CloudFlare.

@adeebshihadeh
Copy link
Contributor

Should be fixed now if you rebase.

Copy link

github-actions bot commented Jun 6, 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://487.connect-d5y.pages.dev

@RossGGG
Copy link
Author

RossGGG commented Jun 6, 2024

Should be fixed now if you rebase.

It seems to be building now, but the built preview does not reflect/include the changes from the PR.

@adeebshihadeh
Copy link
Contributor

Should be fixed now if you rebase.

It seems to be building now, but the built preview does not reflect/include the changes from the PR.

Should be fixed now if you rebase.

@RossGGG
Copy link
Author

RossGGG commented Jun 6, 2024

@adeebshihadeh It looks like the test is failing now, I think because there is no demo data in the default date range when the UI loads now. The preview deployment does seem to have updated with the PR changes, so I will post it on the Discord.

RossGGG added 2 commits June 6, 2024 16:21
* Disabled the column header menus on the fild download columns since they caused formatting issues when they appear, and they don't have much practical use on those columns
* Disabled cell focus box appearing on headers.
@adeebshihadeh
Copy link
Contributor

I'm closing out this bounty, since we're shifting our focus over to new-connect.

The work is done though, so you're good to collect the bounty. Instructions here: https://github.com/commaai/openpilot/blob/master/docs/BOUNTIES.md

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 raw files popup with a table
3 participants