Add a Grid Spacing option to the GridSettings

[Craggar]

Copying styles/elements from the GridSize picker, this adds a GridSpacing picker

Fixes #774

[juliandescottes]

Thanks for opening the PR! I'll have a look today.

[juliandescottes]

Works really nicely, great job!

One comment about the feature itself: it would be nice to change computeGridWidthForDisplay_ so that it keeps displaying the grid a bit longer if grid spacing is enabled. If you use a big spacing with a 320x320 sprite for instance, the grid disapears very quickly when zooming out.

The purpose of the computeGridWidthForDisplay_ method is to ensure that the grid doesn't become too dense or too thick.

Right now it does that with a magical while (this.zoom < 6 * gridWidth) { gridWidth--; }. This works well for thickness. But for density, if spacing is enabled we can keep displaying a thin grid on smaller zoom levels. Something like that would work:

  ns.FrameRenderer.prototype.computeGridWidthForDisplay_ = function () {
    var gridSpacing = this.getGridSpacing();
    if (this.zoom * gridSpacing < 6) {
      return 0;
    }

    var gridWidth = this.getGridWidth();
    while (gridWidth > 1 && this.zoom < 6 * gridWidth) {
      gridWidth--;
    }

    return gridWidth;
  };

[juliandescottes]

Looks good! I think we can avoid creating a new widget but otherwise that's a really nice changeset, thanks for following the conventions in place, even though they're not written anywhere :)

[juliandescottes]

"," should be "{"

[juliandescottes]

Same comment

[juliandescottes]

You don't need to add that, this was some migration code for version 12, because the GRID_WIDTH setting used to accept the "0" value. I should remove this method now :)

[juliandescottes]

I think we could reuse the SizePicker widget directly. The issue is mostly with the hardcoded "4" used in setSpacing. The goal was to represent the maximum value that can be represented via a [data-size] item.

Instead of doing that we can change the logic to do:

• get the element for the provided size this.container.querySelector('[data-size="' + size + '"]')
• if the element is missing, get the last element and add a label:

      selectedOption = this.container.querySelector('[data-size]:last-child');
      selectedOption.classList.add('labeled');
      selectedOption.setAttribute('real-size', size);

With this small change we can reuse the SizePicker directly, just need to change the setSpacing to setSize and the data-spacing to data-size where appropriate.

[Craggar]

Grand, thanks for the feedback - I should get to it today or tomorrow.

[Craggar]

I've made the changes above, and removed the now-redundant SpacingPicker js/css to clean up the PR.

[juliandescottes]

Thanks for addressing the comments! I have two small nits, but I'll just push them in a follow up.

Thanks again for picking this up, I should take some time to make a release now and make this feature available on piskelapp.com!

[juliandescottes]

We still need to have custom sizes defined in CSS for those, otherwise the last four elements have the same size.

[juliandescottes]

variable is not used