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

Make id's uniquer to prevent duplicate id attributes #96

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

Conversation

Frique
Copy link
Contributor

@Frique Frique commented Aug 26, 2013

Options and Optgroups with the same name in different multiselect
instances would get the same ID attribute which is not HTML friendly.

Options and Optgroups with the same name in different multiselect
instances would get the same ID attribute which is not HTML friendly.
@lou
Copy link
Owner

lou commented Aug 29, 2013

Hi @Frique,

First of all, your help on this project is very much appreciated.
Be sure that I will have a close look on each one of your pull requests but I currently have a big load of work on my other projects.

Did you notice that there are some tests on the project? They are written with Jasmine. If you need any help with this test framework do not hesitate to contact me.
It would be great if you add some tests when you add a feature. Note that I didn't test everything so feel free to add more tests on existing features if you fancy it 😄

For this pull request:

  • Tests are failing ! It's not a big deal but I think it would be a good way for you to jump in the specs vortex!
  • I think it would be a better idea to concat the multiselect ID to the elements ID instead of the this.unique call. I did the random ID generation in case the ID is missing but it doesn't handle the ID collision issue very well at all.

Thanks again,

@Frique
Copy link
Contributor Author

Frique commented Aug 29, 2013

Hey, thanks for your feedback. And yes, I am inexperienced with and scared of test frameworks :)
So far I am pushing features and bugfixes that I need it to have in a current project, realizing that it can be a lot to review or can even clash with your own version progress, but hopefully useful in their current or modified state.

  • I will go check out Jasmine, run the tests and commit updates
  • True, i'll change that.

Thanks!

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