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

Removed ids on forms, switched to class selectors and scoped all jquery selects to their own template #360

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kevb
Copy link

@kevb kevb commented Mar 27, 2015

I'm trying to use this module for some off-label purposes (using the templates inside bootstrap modal, as well as the auth route pages) . As part of that, I needed to clean up some stuff so that more than one instance of the sign in form can exist in the DOM at a time (e.g. a hidden version which could be shown as a bootstrap modal).

Here's the changes that I've made. Hopefully it's not too controversial and you can accept my PR:

  1. Forms are referenced by ID (#signIn etc). Since we shouldn't assume anything about the enclosing project's layouts and other html, and since IDs must be unique, it's better to reference by class rather than just occupy a rather generic named id. So I removed the IDs and switched the selectors to classes instead.

  2. There are some instances where template events are getting the value of an input using a very general selector and with global scope, e.g.

    password = $('input[type="password"]').val()

    So if any other exists in the layout for example, there will be problems. This would be much better scoped to the template, like so:

password = template.$('input[type="password"]').val()

Hope this makes sense.

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.

1 participant