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

Assign class selector for modals #682

Open
mjmaix opened this issue Sep 24, 2014 · 10 comments
Open

Assign class selector for modals #682

mjmaix opened this issue Sep 24, 2014 · 10 comments
Labels

Comments

@mjmaix
Copy link

mjmaix commented Sep 24, 2014

I'm using v2.0.2 and the selector for modal is for a tags. I'm using router.js for a single page application and it uses links to switch views. The problem is this error is always thrown

Uncaught SyntaxError: Failed to execute 'querySelector' on 'Document': '#issues/0' is not a valid selector. ratchet.js:37getModal ratchet.js:37(anonymous function)

It does not change the router.js behavior but I think this is a good thing to have..

Please let me know if you have ideas for the selector name and if this should be done, I'm planning to submit a PR if needed.

@mdonstad
Copy link

I second this This wont work with any javascript router that utilizes hash for single page application. I hoping something can be done to make this independent of router libraries

@miguelcobain
Copy link

Trying to use Ratchet with EmberJS. Came across this issue.

👍

@cvrebert cvrebert added the js label Oct 29, 2014
@arturocr
Copy link

arturocr commented Nov 7, 2014

I'm having the same problem with my Backbone app, it doesn't interferes with the router behavior, but it's annoying to have those errors in the log...

@gregates
Copy link

I dislike this solution. Hijacking the href attribute as a container for a css selector (instead of an href) is asking for conflicts with other libraries (not to mention the html spec), even if you change that selector to a class instead of an id. Would strongly prefer a solution with custom attributes, something more bootstrap like perhaps:

<a class="btn" data-target="#myModalExample" data-toggle="modal">Open modal</a>

Popovers could use the same treatment, and then they wouldn't have to be limited to the title bar.

You could go further and make modal/popover triggers entirely indicated by data attrs, so that you could use a button or other element if you wanted.

Obviously that would be a breaking change, but IMO a substantial improvement. If desired, I could submit a PR.

@miguelcobain
Copy link

I can only see advantages, so I agree it is an improvement.
Em 10/11/2014 20:57, "Greg Gates" notifications@github.com escreveu:

I dislike this solution. Hijacking the href attribute as a container for
a css selector (instead of an href) is asking for conflicts with other
libraries (not to mention the html spec
http://www.w3.org/TR/html5/text-level-semantics.html#the-a-element),
even if you change that selector to a class instead of an id. Would
strongly prefer a solution with custom attributes, something more bootstrap
like perhaps:

Open modal

Popovers could use the same treatment, and then they wouldn't have to be
limited to the title bar.

You could go further and make modal/popover triggers entirely indicated by
data attrs, so that you could use a button or other element if you wanted.

Obviously that would be a breaking change, but IMO a substantial
improvement. If desired, I could submit a PR.


Reply to this email directly or view it on GitHub
#682 (comment).

@mohamedbenjelloun
Copy link

Same with AngularJS router.

@juriejan
Copy link

+1 for @gregates suggestion.

@jcsogo
Copy link

jcsogo commented Dec 28, 2014

@juriejan I have patched locally my copy with your PR #730 and it works great.

@arturocr
Copy link

@juriejan's fix worked perfectly for me too! Thanks!

@juriejan
Copy link

juriejan commented Jan 3, 2015

Thanks for the feedback. Added backwards compatibility to the PR today. Might be worth testing again. Worked fine for me though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
9 participants