-
Notifications
You must be signed in to change notification settings - Fork 22
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
Added Example Code to "datalistener.md" #34
base: main
Are you sure you want to change the base?
Conversation
While I do not know your conventions in this context, I would be happy to get feedback to improve this contribution.
removed accidenttally added call to ViewModel.Start() in DataListener()
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.
Thanks for this. We try to go for minimal code samples - perhaps this would be clearer without the ViewModel
and the surrounding unit test code?
For example vm.IsStarted
could just be a local variable to a simple method that sets up the listener.
inlined and simplified ViewModel - renamed isStarted to isRunning to indicate that it is not reflecting the state of the StartButton, but the other way round - removed testing-code - externalized the handleTap-func for the demo's main purpose it could actually be empty, without it feels incomplete, with it the scope is extended - I am unsure.
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.
Much clearer thanks.
Noted an unused import.
Also I would recommend removing blank lines at beginning and end of a function
removed empty lines - I personally prefer at least the opening empty line, to make the function's signature stand out. - the closing one, I missed - the import "testing", was a copy-paste leftover In addition I renamed the function, to better describe its intent. Thanks for your feedback
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.
It looks like there is still an unused test import but you marked that as resolved?
While I do not know your conventions in this context, I would be happy to get feedback to improve this contribution.