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

elegant but naughty toggle-on-property in Show/ Hide comments #44

Open
morkeltry opened this issue Dec 8, 2017 · 0 comments
Open

elegant but naughty toggle-on-property in Show/ Hide comments #44

morkeltry opened this issue Dec 8, 2017 · 0 comments
Assignees

Comments

@morkeltry
Copy link

In comments.js, the toggling listener starting at line 51 gets its own state by examining textContent.
Which is a nice quick way to avoid extra code, but is viewed as bad practice, compared to having a state toggle. Whether or not it actually is in this case is a matter for debate ;)
In code with more folks working on it, the textContent may end up being changed without knowing that this functionality depends on it.
It may also hold a wrong status in unusual cases, such as before your async function has called back, or if there is an error. Again though, in this case, not so important, since you display 'hide comments' once there are comments loaded to hide.

@Dragomirc Dragomirc self-assigned this Dec 8, 2017
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

No branches or pull requests

2 participants