Collapsible groups #534
Collapsible groups #534
Conversation
|
@mistic100 It looks like travis failed due to an error installing sass |
|
I will change grunt-contrib-sass to grunt-sass during the week. In order to use node-sass and remove the dependency to Ruby. Nice effort though, this kind of error with Travis always bothers me. |
|
@jvincilione I switched everything to node-sass 81175dd |
|
Nice, looks like everything passed! |
|
Why did you merged your own branch into itself ? When doing a rebase you have to do a force push, instead you get this horrible history. You can also use interractive rebase to squash your commits. |
|
Sorry, I'll try and fix it. The merge happened because it wouldn't let me push without pulling down first. I'm not all that familiar with Rebase. |
|
Fixed it. Sorry for the trouble. |
…on and a collapse metadata for it as well.
|
Added naming support #535 |
There was a problem hiding this comment.
Missing the update of the model. See https://github.com/mistic100/jQuery-QueryBuilder/blob/dev/src/plugins/not-group/plugin.js#L107
src/public.js
Outdated
| @@ -237,6 +237,14 @@ QueryBuilder.prototype.getRules = function(options) { | |||
| groupData.data = $.extendext(true, 'replace', {}, group.data); | |||
| } | |||
|
|
|||
| if (typeof group.collapsed !== 'undefined') { | |||
There was a problem hiding this comment.
this code must no be in the core. use the groupToJson event from the plugin
|
Made the changes. Let me know if there are any other issues. |
|
@mistic100 Found a bug with loading regarding the collapse stuff and fixed it. Let me know if there is anything else you want me to do here... |
|
Yes i have other reviews. Sorry for taking so long, my free time is sparse. |
| }); | ||
|
|
||
| // Modify templates | ||
| this.on('getGroupTemplate.filter', function(h, level) { |
There was a problem hiding this comment.
for performances, merge the two handlers, and but the "if" here
| * Save the entered group name on the group object | ||
| * @param {jQuery Element} [$el] | ||
| */ | ||
| setGroupName: function($el) { |
There was a problem hiding this comment.
This code should be in the event handler directly, the function itself is useless.
| * @param {jQuery Element} [$el] | ||
| * @param {object} [options] {@link module:plugins.Collapse} | ||
| */ | ||
| collapse: function($el, options) { |
There was a problem hiding this comment.
The first parameter should be a Group object.
| * @param {jQuery Element} [$el] | ||
| * @param {object} [options] {@link module:plugins.Collapse} | ||
| */ | ||
| collapse: function($el, options) { |
There was a problem hiding this comment.
The method should be named toggleGroup
| * @param {jQuery Element} [$el] | ||
| * @param {object} [options] {@link module:plugins.Collapse} | ||
| */ | ||
| collapse: function($el, options) { |
There was a problem hiding this comment.
Instead of having to pass the options use this.getPluginOptions('collapse-group')
|
|
||
| // Collapse any groups that were saved as collapsed | ||
| this.on('afterSetRules', function() { | ||
| $.each($(Selectors.group_container), function(i, el) { |
There was a problem hiding this comment.
You iterate over ALL groups of ALL builders in the page, this will break.
use this.getModel().each() instead which iterates over the builder model (see to jsdoc for recursive traversal)
| * @param {object} [options] {@link module:plugins.Collapse} | ||
| */ | ||
| collapse: function($el, options) { | ||
| var self = this; |
| self.collapse($(el).find('[data-collapse="group"]'), options); | ||
| } | ||
| if (group.name) { | ||
| $(el).find('.group-name').val(group.name); |
There was a problem hiding this comment.
This code should not be here, remove it and use a model event
this.model.on('update', function(e, node, field, value) {
if (node instanceof Group && field === 'name') {
node.$el.find('.group-name').val(node.name);
}
});| namedGroups: true | ||
| }); | ||
|
|
||
| /** |
There was a problem hiding this comment.
To be separated in two distinct comments blocks for proper doc generation
| * @param {object} [options] | ||
| * @param {string} [options.iconUp='glyphicon glyphicon-chevron-up'] | ||
| * @param {string} [options.iconDown='glyphicon glyphicon-chevron-down'] | ||
| */ |
|
@jvincilione Sorry the review was not published, I didn't see it because the behavior changed some months ago |
…-list.
Merge request checklist
devand I am issuing the PR todev__localeand__authorfieldsLooks like this solves #347