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

Association Filters #44

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

jonmifsud
Copy link
Member

Adds support for Association Filters when using the Association UI. The filters are provided through the symphony core in javascript, and are sent through ajax requests for data when using autocomplete.

var updateFilters = function(id,newfilters) {
fields.each(function(){
var field = $(this);
if (field.attr('id') == "field-" + id){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use === . Always.

@nitriques
Copy link

Great job! I do not want to be mean, but I've reviewed your code ;)

@jonmifsud
Copy link
Member Author

Thanks actually I was expecting more changes. I'll try run update them today/tomorrow.

@nitriques
Copy link

Thanks!

@nathanhornby
Copy link
Contributor

Hey @nitriques - do you run a code reviewing service? Could do with a more involved jshint replacement :)

@nitriques
Copy link

do you run a code reviewing service?

Nope! But we do use scrutinizer for the core, but results are mitigated because we did not used composer before. Should be better since we start using it.

As for jshint, I did not found any good service for it, except travis-ci (which could run a grunt task)...

If you want to send a PR with a configured jshint task, I would put it on travis for you ;)

@nilshoerrmann
Copy link
Contributor

I'm not up-to-date with the Symphony core development: is there anything needed on that side to make this work or can we just pull this in?

@jonmifsud: Would you mind updating your pull request based on @nitriques comments? Thanks!

@jonmifsud
Copy link
Member Author

Been planning to for a while. I'm a bit tight these next couple of days,
I'll do it as soon as I have a couple of minutes to spare.

On Thu, 2 Apr 2015 at 14:08 Nils Hörrmann notifications@github.com wrote:

I'm not up-to-date with the Symphony core development: is there anything
needed on that side to make this work or can we just pull this in?

@jonmifsud https://github.com/jonmifsud: Would you mind updating your
pull request based on @nitriques https://github.com/nitriques comments?
Thanks!


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

@nilshoerrmann
Copy link
Contributor

Cool, thanks!

@nitriques
Copy link

@nilshoerrmann If you could test it, it would be nice ;)

@nilshoerrmann
Copy link
Contributor

That was my plan ;)

@nitriques
Copy link

Thanks Nils!

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

4 participants