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

Example app update #259

Closed
wants to merge 5 commits into from
Closed

Example app update #259

wants to merge 5 commits into from

Conversation

Hung6129
Copy link

@Hung6129 Hung6129 commented Jan 2, 2023

No description provided.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hello @Hung6129 ,
Thanks for the PR 👍
A few changes before we merge..

example/lib/src/views/control_module/node.dart Outdated Show resolved Hide resolved
example/lib/src/views/home/home.dart Outdated Show resolved Hide resolved
example/lib/src/views/home/home.dart Outdated Show resolved Hide resolved
lib/src/utils/provisioning.dart Outdated Show resolved Hide resolved
@Hung6129
Copy link
Author

Hung6129 commented Jan 4, 2023

Oh, about the node name
I think do it earlier it good too
It just i do not know a efficient way to do this 😢
If in the provisioning method where should i call it tho and same with UnprovisionedMeshNode object?
hope you can show me where is the best way to call and expose it
thanks

@ghost ghost changed the title Update resolve conflict Example app update Jan 11, 2023
@ghost
Copy link

ghost commented Jan 11, 2023

Hi @Hung6129 ,

Changing the name ealier probably implies to edit some native code and I couldn't find it easily on GitHub to guide you. Maybe we can split your changes : revert the changes on the plugin and only keep changes in example app. If you follow the comments I added then we can merge the example app update and work on choosing name upon provisioning in a separate PR ?

@Hung6129
Copy link
Author

Hung6129 commented Jan 12, 2023

Yeah sure !!
Can you tell me when we can start?
And what can i do to make it easier for you?

Btw it was mine first time to create a PR on a repo, hope u can skip out on the mistake 😟

Thanks you

@ghost
Copy link

ghost commented Jan 12, 2023

You didn't make any mistakes. Thanks for your contribution 😃

Make a commit on your branch that applies my suggestions and that removes the name change at the end of provisioning. So we can merge the example app update.
Then open a new issue for the feature request about having the newly provisioned node named after the given DiscoveredDevice.

@ghost
Copy link

ghost commented Jan 13, 2023

Thx for the removal and for other updates in the issues 👍
There are still some needed changes before we can merge 🚀

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hello @Hung6129

Would you have some time to apply my suggested changes ?
So we can merge your PR ;)

example/lib/src/views/home/home.dart Outdated Show resolved Hide resolved
Copy link
Author

@Hung6129 Hung6129 left a comment

Choose a reason for hiding this comment

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

Resolve the changes

lib/src/utils/provisioning.dart Outdated Show resolved Hide resolved
example/lib/src/views/control_module/node.dart Outdated Show resolved Hide resolved
example/lib/src/views/home/home.dart Outdated Show resolved Hide resolved
example/lib/src/views/home/home.dart Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks 😊 LGTM except the pubspec.lock files update as it implies higher dart sdk constraint

pubspec.lock Outdated Show resolved Hide resolved
example/pubspec.lock Outdated Show resolved Hide resolved
@Hung6129 Hung6129 closed this by deleting the head repository May 3, 2024
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.

None yet

1 participant