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鈥檒l 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

@zaygraveyard zaygraveyard commented Aug 29, 2016

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 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

@zaygraveyard zaygraveyard commented Aug 29, 2016

Shall I submit a PR?

@NeXTs
Copy link
Owner

@NeXTs 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

@zaygraveyard zaygraveyard commented Aug 29, 2016

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 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 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 in 6a16567 Sep 12, 2016
@NeXTs
Copy link
Owner

@NeXTs 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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can鈥檛 perform that action at this time.