getgrav / grav-plugin-simplesearch

Grav SimpleSearch Plugin
https://getgrav.org
MIT License
44 stars 55 forks source link

Removed jQuery dependency #57

Closed Swader closed 7 years ago

Swader commented 8 years ago

With jQuery required so explicitly, this pretty much forces the user to have assets at the top of the file, else jQuery will be undefined. By removing the dependency on jQuery, this is still cross-browser compatible but now allows for JS assets to be output anywhere else in the page.

rhukster commented 8 years ago

Thanks for this. I'll take a look!!

flaviocopes commented 8 years ago

I don't mind removing the jQuery dependency, but another option is to instead load the JS in its own file, and assign a priority high enough, to suit the priority that most themes give to jQuery.

flaviocopes commented 8 years ago

And also we should probably have some convention / guidelines for priorities to assign to base libraries, and user JS files?

Swader commented 8 years ago

Indeed. To be honest, I don't understand why the JS was even necessary there, or why Grav insists on using this weird query param notation. If it used a normal query param, it could all be done with no JS at all, just a simple form. Any specific reason why the search can't work with something like search/query/whatever, or search/?query=whatever as opposed to search/query:whatever?

rhukster commented 8 years ago

The query params style attributes is purely for 'prettyness and simplicity' really and a more friendly approach as opposed to regular params with an initial ? and subsequent & characters to create ?foo=bar&baz=qux, you can simply use /foo:bar/baz:qux syntax. I saw this first in Statamic and really liked it, so kinda borrowed it for Grav.

However, Grav still supports regular queries and if it doesn't already we can easily check both Uri::param() and Uri::query() to support both simultaneously.

Swader commented 8 years ago

It didn't work with the regular query params, no, I tried. I think it should support regular query params as well (check for both), but maybe prioritize one over the other, to accommodate all users.

rhukster commented 8 years ago

https://github.com/getgrav/grav-plugin-simplesearch/issues/60

Added an issue for this

rhukster commented 7 years ago

Now I just need to test!

flaviocopes commented 7 years ago

Thanks @Swader, finally merged