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

Use event handler instead of 'javascript:fn' href #55

Closed
wants to merge 1 commit into from

Conversation

prtksxna
Copy link
Contributor

@prtksxna prtksxna commented Aug 7, 2016

No description provided.

@aaronpk
Copy link
Owner

aaronpk commented Dec 21, 2016

What's the reason for this change?

@cweiske
Copy link
Contributor

cweiske commented Dec 21, 2016

I would not merge this. It adds more indirection, and makes the code harder to understand.

@aaronpk aaronpk closed this Jan 6, 2017
@prtksxna
Copy link
Contributor Author

prtksxna commented Jan 9, 2017

Oops! I somehow missed notifications from this repo. There are a couple of reasons for the change:

  • Separation of content (HTML) and behavior/action (JS): Its similar to why we don't write CSS in the style attribute of elements.

  • No mixing of languages: I am hoping to submit further patches to separate the JS files so that we are able to lint and test them separately.

  • Consistency: The <script> tag at the bottom already attaches events to the DOM elements.

@prtksxna
Copy link
Contributor Author

Hey @aaronpk, are you planning to look into this again?

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.

3 participants