-
Notifications
You must be signed in to change notification settings - Fork 25
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
Refactor robot example #162
Refactor robot example #162
Conversation
the missing await here was causing an exception to get propogated when it should've been caught and handled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, but LGTM overall
if (dotenv.env['ROBOT_LOCATION'] != null && dotenv.env['LOCATION_SECRET'] != null) { | ||
robotFut = RobotClient.atAddress( | ||
dotenv.env['ROBOT_LOCATION'] ?? '', | ||
RobotClientOptions.withLocationSecret(dotenv.env['LOCATION_SECRET'] ?? ''), | ||
); | ||
} else if (dotenv.env['API_KEY_ID'] != null && dotenv.env['API_KEY'] != null) { | ||
robotFut = RobotClient.atAddress( | ||
dotenv.env['ROBOT_LOCATION'] ?? '', // or whatever default value you want | ||
RobotClientOptions.withApiKey( | ||
dotenv.env['API_KEY_ID'] ?? '', | ||
dotenv.env['API_KEY'] ?? '', | ||
), | ||
); | ||
} else { | ||
throw Exception('None of the required variables are defined in .env. Please see README.md for more information.'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe we should keep this in, as it allows a user to interact using either the API Key or the Location Secret. Unless we want to deprecate the use of location secrets entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding all the examples of using Location Secrets have been removed from the App side, I think it makes sense to remove from SDKs too.
@@ -223,7 +172,7 @@ class _MyHomePageState extends State<MyHomePage> { | |||
Text(_robot.resourceNames.where((element) => element.type == resourceTypeComponent).join('\n')), | |||
]) | |||
: _loading | |||
? PlatformCircularProgressIndicator() | |||
? const CircularProgressIndicator() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps use CircularProgressIndicator.adaptive()
to keep it more native (since you're removing all the platform native stuff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok i'll add that. Just trying to simplify the examples as much as possible. .adaptive()
on any standard flutter widgets doesn't feel to intrusive.
I was looking into our example code and noticed some quick improvements.
The largest change is pulling out some packages we used in the robot example - this simplifies the code and we're only using vanilla flutter widgets now so it should be more clear to the average reader what is unique to our package and what is not.
Was looking into a mDNS dial failure i was getting and realized we were missing an
await
in the mDNS flow.Also got our Podfile all pointing to the same iOS minimum version & included mDNS entitlements in the robot example info.plist.
tested locally and both examples still compile and run for me.