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 jQuery dependency #57

Merged
merged 3 commits into from Dec 19, 2016
Merged

Removed jQuery dependency #57

merged 3 commits into from Dec 19, 2016

Conversation

Swader
Copy link
Contributor

@Swader Swader commented Aug 6, 2016

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.

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.
Small bugfix with references to values inside closures.
@rhukster
Copy link
Member

rhukster commented Aug 6, 2016

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

Avoid checking for the search button if the setting says not to show it.
@flaviocopes
Copy link
Contributor

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
Copy link
Contributor

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

@Swader
Copy link
Contributor Author

Swader commented Aug 8, 2016

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
Copy link
Member

rhukster commented Aug 8, 2016

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
Copy link
Contributor Author

Swader commented Aug 8, 2016

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
Copy link
Member

rhukster commented Aug 8, 2016

#60

Added an issue for this

@rhukster
Copy link
Member

Now I just need to test!

@flaviocopes flaviocopes merged commit 529d094 into getgrav:develop Dec 19, 2016
@flaviocopes
Copy link
Contributor

Thanks @Swader, finally merged

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.

None yet

3 participants