-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 heavier selector for select2 #1997
Conversation
Looks like a reasonable change. We have a test failure here that looks related. I'd also like to see a better commit description. Thanks for looking in to this! |
Thank you! I'm looking to the failure right now. |
@gmacdougall I've tried but I was unable to replicate the error, it seems that the failure spec fails randomly (at least in my environment). Is it possible? Do you know if someone has a problem like this? With ruby 2.4.0 I was unable to even run the spec because of
And sometimes the spec pass without problems. Do you have any advice to solve this and get the PR merged? Thank you!! |
Should we be using an ID or Should allow us to clean up these selectors even more. |
Yes, I have opted for the |
Wouldn't it be better if we switched to https://github.com/selectize/selectize.js altogether? |
There are some good suggestions here, but I think they're all outside the scope of fixing this immediate issue. Many extensions use I'd love to see us move away from that, to bootstrap's |
Sounds good! 👍 It wasn't fully clear to me why this was necessary so I pulled stuff down and tested things out. I think improving the commit message here will go a long way in clarifying things for future-us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think improving the commit message here will go a long way in clarifying things for future-us.
Yes, please add some more information in the commit message so future readers know why we made this.
Thanks for the contribution
Using a simple class selector to init select2 will throw an error if there is already an instance of select2 in the page with the same class selector.
@tvdeyen I've tried to add the information on the commit, let me know if is fine. I thought I had to stay in the character limit 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. Thanks!
This character only applies to the first sentence. The description body (separated by a new line) can and should be as long as you need to explain the rational behind your changes. A very good read on this topic is: https://chris.beams.io/posts/git-commit/ |
See Issue #1996