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

Added Example Code to "datalistener.md" #34

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

t-hofmann
Copy link

While I do not know your conventions in this context, I would be happy to get feedback to improve this contribution.

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()
Copy link
Member

@andydotxyz andydotxyz left a 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.
Copy link
Member

@andydotxyz andydotxyz left a 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

api/v2.5/data/binding/datalistener.md Show resolved Hide resolved
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
@t-hofmann t-hofmann requested a review from andydotxyz October 31, 2024 09:58
Copy link
Member

@andydotxyz andydotxyz left a 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?

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