Enforce minItems / maxItems #22

Closed
dfliess opened this Issue Nov 11, 2012 · 6 comments

Comments

Projects
None yet
2 participants
@dfliess

dfliess commented Nov 11, 2012

"minItems": X,
"maxItems":Y

Is not working on array.

It works maxLength on tabarray, but I think that maxItems should be used.

@tidoust

This comment has been minimized.

Show comment Hide comment
@tidoust

tidoust Nov 12, 2012

Contributor

Hi @dfliess,

I count at least two bugs and a couple of missing features:

  1. The maxLength for arrays is maxItems. Oops.
  2. maxItems is not supported on array fields for the time being
  3. Current code that checks maxLength for tabarray fields can easily crash JSON Form because it takes for granted that the first input field in the tabarray must be the first great-child of the tabarray field definition. That's often the case, but not a guarantee.
  4. minItems is not supported

I'll fix the first 3 points.
Support for minItems is a good idea but not a high priority for me at the moment. I'll see if it can be added easily. Feel free to contribute code!

Contributor

tidoust commented Nov 12, 2012

Hi @dfliess,

I count at least two bugs and a couple of missing features:

  1. The maxLength for arrays is maxItems. Oops.
  2. maxItems is not supported on array fields for the time being
  3. Current code that checks maxLength for tabarray fields can easily crash JSON Form because it takes for granted that the first input field in the tabarray must be the first great-child of the tabarray field definition. That's often the case, but not a guarantee.
  4. minItems is not supported

I'll fix the first 3 points.
Support for minItems is a good idea but not a high priority for me at the moment. I'll see if it can be added easily. Feel free to contribute code!

tidoust pushed a commit that referenced this issue Nov 12, 2012

François Daoust
Support for maxItems and minItems (partial) added for arrays (#22)
Changes and fixes:
- JSON Form now correctly supports "maxItems" for arrays instead of
"maxLength". "maxLength" is still supported as fallback for backward
compatibility purpose.
- All array fields ("array" and "tabarray") now support "maxItems"
- The code correctly checks constraints on the number of items within
an array no matter how the form is structured.
- "minItems" is partially supported: it is not enforced at init phase,
meaning that the arrays are not created with the right minimum number
of items. The "suppress item" button is correctly disabled otherwise.
@tidoust

This comment has been minimized.

Show comment Hide comment
@tidoust

tidoust Nov 12, 2012

Contributor

Done for first 3 points. I also added partial support for minItems but that's not yet usable because the constraints is not yet enforced when the form is initialized.

Contributor

tidoust commented Nov 12, 2012

Done for first 3 points. I also added partial support for minItems but that's not yet usable because the constraints is not yet enforced when the form is initialized.

dfliess pushed a commit to dfliess/jsonform that referenced this issue Nov 12, 2012

dfliess pushed a commit to dfliess/jsonform that referenced this issue Nov 14, 2012

@dfliess

This comment has been minimized.

Show comment Hide comment
@dfliess

dfliess Nov 14, 2012

Hi @tidoust

I've commited some changes at my fork that you can check. There was an issue when deleting an item other than the upper bound of a tabarray.
Also I added a first approach to enforce minItems on form initialize, but I'm having one issue that updateTabs() is not selecting the first tab and it's not being rendered, but if you click the tab it's being rendered ok.
It would be great if you could check it, if you want I can make a Pull request anyway.

dfliess commented Nov 14, 2012

Hi @tidoust

I've commited some changes at my fork that you can check. There was an issue when deleting an item other than the upper bound of a tabarray.
Also I added a first approach to enforce minItems on form initialize, but I'm having one issue that updateTabs() is not selecting the first tab and it's not being rendered, but if you click the tab it's being rendered ok.
It would be great if you could check it, if you want I can make a Pull request anyway.

dfliess pushed a commit to dfliess/jsonform that referenced this issue Nov 14, 2012

@tidoust

This comment has been minimized.

Show comment Hide comment
@tidoust

tidoust Nov 16, 2012

Contributor

Nice!

Good point on deletion index. For symmetry, it's would actually be a good idea to use node.children.length instead of idx for checks on maxItems. Right now, insertion creates an item at the end of an array or tabarray (meaning idx === node.children.length in practice for insertions), but we could imagine that it actually inserts the item right at the position of the active tab in a tabarray in the future. If you can make the change, that's great, no big deal otherwise.

Re. updateTabs not playing nicely with you, that's because:

  1. during form rendering, click events on tab arrays are not yet bound (done at the end by the call to initializeTabs)
  2. tabs created through a call to insertArrayItem are not flagged as active

In the end, the markup is correct but the final call to initializeTabs fails to detect which tab is active as none of them is flagged as such. That is, the following line does nothing:

$(this).find('ul.nav li.active').click();

The workaround is to force the active flag on the first tab in updateTabs when it's called during initialization. The following code should do the trick (difference with current code are lines that define/use the activateFirstTab variable):

      var updateTabs = function (selIdx) {
        var tabs = '';
        var activateFirstTab = false;
        if (selIdx === undefined) {
          selIdx = $('> .tabbable > .nav-tabs .active', $nodeid).data('idx');
          if (selIdx) {
            selIdx = parseInt(selIdx, 10);
          }
          else {
            activateFirstTab = true;
            selIdx = 0;
          }
        }
        if (selIdx >= node.children.length) {
          selIdx = node.children.length - 1;
        }
        _.each(node.children, function (child, idx) {
          var title = child.legend ||
            child.title ||
            ('Item ' + (idx+1));
          tabs += '<li data-idx="' + idx + '">' +
            '<a class="draggable tab" data-toggle="tab">' +
            escapeHTML(title) +
            '</a></li>';
        });
        $('> .tabbable > .nav-tabs', $nodeid).html(tabs);
        if (activateFirstTab) {
          $('> .tabbable > .nav-tabs [data-idx="0"]', $nodeid).addClass('active');
        }
        $('> .tabbable > .nav-tabs [data-toggle="tab"]', $nodeid).eq(selIdx).click();
      };

If you can integrate these changes and make a pull request, I'd be happy to merge it.

Contributor

tidoust commented Nov 16, 2012

Nice!

Good point on deletion index. For symmetry, it's would actually be a good idea to use node.children.length instead of idx for checks on maxItems. Right now, insertion creates an item at the end of an array or tabarray (meaning idx === node.children.length in practice for insertions), but we could imagine that it actually inserts the item right at the position of the active tab in a tabarray in the future. If you can make the change, that's great, no big deal otherwise.

Re. updateTabs not playing nicely with you, that's because:

  1. during form rendering, click events on tab arrays are not yet bound (done at the end by the call to initializeTabs)
  2. tabs created through a call to insertArrayItem are not flagged as active

In the end, the markup is correct but the final call to initializeTabs fails to detect which tab is active as none of them is flagged as such. That is, the following line does nothing:

$(this).find('ul.nav li.active').click();

The workaround is to force the active flag on the first tab in updateTabs when it's called during initialization. The following code should do the trick (difference with current code are lines that define/use the activateFirstTab variable):

      var updateTabs = function (selIdx) {
        var tabs = '';
        var activateFirstTab = false;
        if (selIdx === undefined) {
          selIdx = $('> .tabbable > .nav-tabs .active', $nodeid).data('idx');
          if (selIdx) {
            selIdx = parseInt(selIdx, 10);
          }
          else {
            activateFirstTab = true;
            selIdx = 0;
          }
        }
        if (selIdx >= node.children.length) {
          selIdx = node.children.length - 1;
        }
        _.each(node.children, function (child, idx) {
          var title = child.legend ||
            child.title ||
            ('Item ' + (idx+1));
          tabs += '<li data-idx="' + idx + '">' +
            '<a class="draggable tab" data-toggle="tab">' +
            escapeHTML(title) +
            '</a></li>';
        });
        $('> .tabbable > .nav-tabs', $nodeid).html(tabs);
        if (activateFirstTab) {
          $('> .tabbable > .nav-tabs [data-idx="0"]', $nodeid).addClass('active');
        }
        $('> .tabbable > .nav-tabs [data-toggle="tab"]', $nodeid).eq(selIdx).click();
      };

If you can integrate these changes and make a pull request, I'd be happy to merge it.

@dfliess

This comment has been minimized.

Show comment Hide comment
@dfliess

dfliess Nov 16, 2012

@tidoust,

Already merged and creating Pull Request, also fixed a small thing when enforcing minItems to disable delete button (on arrays and tabarrays). Please test it again before merging :D

dfliess commented Nov 16, 2012

@tidoust,

Already merged and creating Pull Request, also fixed a small thing when enforcing minItems to disable delete button (on arrays and tabarrays). Please test it again before merging :D

dfliess pushed a commit to dfliess/jsonform that referenced this issue Nov 16, 2012

@tidoust

This comment has been minimized.

Show comment Hide comment
@tidoust

tidoust Nov 16, 2012

Contributor

@dfliess Looks good, thanks!

I just merged the pull request.

Contributor

tidoust commented Nov 16, 2012

@dfliess Looks good, thanks!

I just merged the pull request.

@tidoust tidoust closed this Nov 16, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment