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

[RSDK-4265] Widgets #78

Merged
merged 5 commits into from
Aug 1, 2023
Merged

[RSDK-4265] Widgets #78

merged 5 commits into from
Aug 1, 2023

Conversation

njooma
Copy link
Member

@njooma njooma commented Jul 31, 2023

Adding widgets to the flutter sdk. Eventually this will be built out more, so I will be using this branch to add more widgets and merge them in.

@njooma njooma requested a review from a team as a code owner July 31, 2023 21:52
iconTrailing;
}

class ViamButton extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

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

what is the use of a viamButton? does this mean rather than someone using an ElevatedButton from flutter and styling it to match the viam theme, then they could just use a viamButton? Or is there greater functionality?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's basically just that -- just a button that's always going to be styled for Viam (regardless of what your theme is). I mostly made this for use internally for any/all buttons in the SDK widgets, and figured we might as well publish it like we do for PRIME.

@@ -1,15 +1,16 @@
import 'package:flutter/material.dart';
import 'package:flutter_platform_widgets/flutter_platform_widgets.dart';
import 'package:viam_sdk/viam_sdk.dart';
import 'package:viam_sdk/widgets.dart';

class BaseScreen extends StatelessWidget {
final Base base;
final ResourceName resourceName;
Copy link
Member

Choose a reason for hiding this comment

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

Do you need resourceName still? is it being used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't. I left it in because all the other screens still have ResourceName. I can remove it.

Copy link
Member

@jckras jckras left a comment

Choose a reason for hiding this comment

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

Added one potential change and the other comments are for my understanding, LGTM

@njooma njooma merged commit c9ced73 into viamrobotics:main Aug 1, 2023
1 check passed
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