Skip to content

RSDK-10304: Button component #363

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

Merged
merged 9 commits into from
Mar 31, 2025
Merged

Conversation

lia-viam
Copy link
Contributor

Generated with Claude

I did not include a ViamButtonWidget like in #348 but could add one if we want it

@lia-viam lia-viam requested a review from a team as a code owner March 21, 2025 14:40
@lia-viam lia-viam requested review from njooma and stuqdog March 21, 2025 14:40
Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

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

One change requested to help avoid flakey tests, otherwise lgtm!

const name = 'button';

setUp(() async {
final port = 50000 + (name.hashCode % 10000);
Copy link
Member

Choose a reason for hiding this comment

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

oh, sad :( we had a helper function that did this but it sometimes led to flakey test results because of ports being already used, so it was replaced with serveServerAtUnusedPort. But, I seem to have missed the cases where we were making the port by hand here instead of calling generateTestingPortFromName. All cases of this final port = 50000 + (name.hashCode % 10000); in our tests should be replaced, though.

I'm torn on whether we should flyby fix all cases of this in this PR, or make a new ticket, so I'll defer to you on that. Either way, at a minimum I think we should definitely use serveServerAtUnusedPort here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, changed it to use the same pattern as in the arm wrapper!

@stuqdog
Copy link
Member

stuqdog commented Mar 24, 2025

I did not include a ViamButtonWidget like in #348 but could add one if we want it

Oh this is an interesting question, seems to me that adding a widget would be good? but I don't feel particularly strongly

@lia-viam lia-viam merged commit a088604 into viamrobotics:main Mar 31, 2025
4 checks passed
@lia-viam lia-viam deleted the feature/button branch March 31, 2025 14:49
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.

3 participants