Skip to content

Conversation

@paytontech
Copy link

adds device sharing & un-sharing device as well as a list of users the device is shared with. here's a demo video:

Screen.Recording.2025-05-06.at.3.59.53.PM.mov

open to feedback, would like to know what to change/fix etc. thanks!

@github-actions
Copy link

github-actions bot commented May 6, 2025

Changes:

path lines diff
./pages/dashboard/activities/SettingsActivity.tsx 452 +47
./api/devices.ts 131 +18

Total lines: 4795 (+65)

@github-actions
Copy link

github-actions bot commented May 6, 2025

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

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

Mobile

Desktop

Copy link
Collaborator

@incognitojam incognitojam left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, let's just fix these things

@paytontech
Copy link
Author

thanks for the feedback! I didn't even notice those APIs were already available, thanks for catching that! I tried using onInput already, but it didn't seem to work. I'll try again, maybe I did something wrong. Agree on the design, it definitely needs some improvement. will work on that, appreciate it

@incognitojam
Copy link
Collaborator

Those methods are in the other comma-api package, but we aren't using that directly because it's written in JS and doesn't have TypeScript type hints

@paytontech
Copy link
Author

Took your design advice, is this any better
Screenshot 2025-05-07 at 10 23 50 AM
?

Copy link
Collaborator

@incognitojam incognitojam left a comment

Choose a reason for hiding this comment

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

It looks better, but I think it needs a little more work

The device settings didn't look nice in the first place, but you can see in this PR (since reverted, will be added again eventually) where I added the device name field and made the text field take up all of the space, with the update button at the end.

commaai/connect#576
https://590.connect-d5y.pages.dev/c1bd6d7f12f35286/settings
image

)}
</For>
<div class="flex items-center gap-2 justify-between">
<TextField
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make the text field fill the entire space instead of leaving a gap between it and the share button

@paytontech
Copy link
Author

got it, thank you!

@incognitojam
Copy link
Collaborator

Oh I think I see the problem you had with adding your own listener - let me have a look at what I did last time

@incognitojam
Copy link
Collaborator

incognitojam commented May 8, 2025

This link should show you how I used the TextField element in the "update device name" PR. Instead of using event listeners to update a signal with the email, you can use a <form> and an action: https://github.com/commaai/connect/pull/576/files#diff-a7d1050a0f5b1fa110b856ccc635b4621bdc0badb012da94d93c3fabc2d63e98R403-R416

Then you can useSubmission to get things like .pending.

@paytontech
Copy link
Author

that's really helpful, I appreciate it! working on doing this now

paytontech added 3 commits May 8, 2025 19:38
importing action from solidjs-router brought bundle size up by a little over 1kb, exceeding max bundle size limit. moved to a classic event handler
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.

2 participants