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

BUG: Vertical margins are not supported #78

Closed
zaygraveyard opened this issue Aug 29, 2016 · 7 comments
Closed

BUG: Vertical margins are not supported #78

zaygraveyard opened this issue Aug 29, 2016 · 7 comments

Comments

@zaygraveyard
Copy link

In the same spirit of react-sortable-hoc#26

Margins are not taken into account in the item_height calculations.

I'll be happy to submit a PR if needed (I already have the fix)

PS: Thanks for the great library 😄

@NeXTs
Copy link
Owner

NeXTs commented Aug 29, 2016

Hm, yes it makes sense

Margins weren't taken into account because they doesn't work for table rows anyway, but this may be useful for div, ul/ol lists

@zaygraveyard
Copy link
Author

Shall I submit a PR?

@NeXTs
Copy link
Owner

NeXTs commented Aug 29, 2016

Sure, if you have time, desire and already working solution.
I'll definitely accept your PR and made additional changes by myself, if needed.

or you could drop a snippet here with required changes and I'll do the rest

I already have an idea where fix should be applied. Additional condition to check margins here using getStyle func

@zaygraveyard
Copy link
Author

Here's the snippet:

    getRowsHeight: function(rows) {
      var opts = this.options,
        prev_item_height = opts.item_height;
      opts.cluster_height = 0
      if( ! rows.length) return;
      var nodes = this.content_elem.children;
      var node = nodes[Math.floor(nodes.length / 2)];
      opts.item_height = node.offsetHeight;
      // consider table's border-spacing
      if(opts.tag == 'tr' && getStyle('borderCollapse', this.content_elem) != 'collapse')
        opts.item_height += parseInt(getStyle('borderSpacing', this.content_elem), 10) || 0;
      // consider margins (and margins collapsing)
      var marginTop = parseInt(getStyle('marginTop', node), 10);
      var marginBottom = parseInt(getStyle('marginBottom', node), 10);
      opts.item_height += Math.max(marginTop, marginBottom);
      opts.block_height = opts.item_height * opts.rows_in_block;
      opts.rows_in_cluster = opts.blocks_in_cluster * opts.rows_in_block;
      opts.cluster_height = opts.blocks_in_cluster * opts.block_height;
      return prev_item_height != opts.item_height;
    },

@NeXTs
Copy link
Owner

NeXTs commented Aug 29, 2016

Thanks!

But now we should additionally take into account margin collapsing, cases when those collapsing are not applied, etc..
I'll investigate it at free time, approximately at these weekend

@zaygraveyard
Copy link
Author

zaygraveyard commented Aug 29, 2016

Happy to help 😄

As for the margin collapsing, I had already investigated this problem before for react-sortable-hoc.
And found that its a very hard problem to solve completely but if we consider that all items have the same margins then the result of margin collapsing will be a simple max operation (as in the snippet).
For more info you can check-out the comments on the PR react-sortable-hoc#27.

Oh and I forgot to mention that I added the following rule to the clusterize.css:

.clusterize-extra-row{
  margin: 0;
}

But I'm not sure if its the best solution.

@NeXTs NeXTs closed this as completed in 6a16567 Sep 12, 2016
@NeXTs
Copy link
Owner

NeXTs commented Sep 12, 2016

Thanks @zaygraveyard for your contribution!

Your PR did all the necessary work, only thing that I needed to add was additional condition to check if it's not table row since margins don't affect them so that calculations should be made accordingly

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

No branches or pull requests

2 participants