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

Items in subgroups incorrectly stacking on initial draw and after setItems #343

Closed
mpilone opened this issue Feb 27, 2020 · 3 comments · Fixed by #360
Closed

Items in subgroups incorrectly stacking on initial draw and after setItems #343

mpilone opened this issue Feb 27, 2020 · 3 comments · Fixed by #360
Labels
bug Something isn't working released

Comments

@mpilone
Copy link
Contributor

mpilone commented Feb 27, 2020

I'm seeing an issue in 7.1.3 with items stacking when they shouldn't. The issue appears on the initial draw of the timeline as well as after calling set items. I attached a test case which is just a modified version of a bundled example.

Steps to reproduce:

  1. Drop the example over the existing one in examples/timeline/items/
  2. Load the example, in my case with Firefox 73.0.1 on macOS 10.14.6
  3. See the stacked items such as BBC (see screenshot)
  4. Drag the timeline and notice that the BBC items stack properly, all in one line.
  5. Click the "setItems" button and noticed that the BBC items stack again.

Expected behavior is that the items/subgroups that abut but don't overlap should be in a continuous row.

I think this is related to subgroups because I don't see it in other places where I don't use subgroups in the timeline but I'm not 100% sure of that.

This might be related to #103 and/or #74 but it isn't clear.

Initial draw stacking

After move correct

expectedVsActualTimesItems.html.zip

@mpilone
Copy link
Contributor Author

mpilone commented Mar 4, 2020

I tracked the problem down to the Stack.collisionByTimes method. It looks like that method does a "less than or equal" comparison so abutting subgroups will be stacked. This is different from the behavior of Stack.collision used for items that only does a "less than" comparison so abutting items remain in the same row.

I modified collisionByTimes to read:

/**
 * Test if the two provided objects collide
 * The objects must have parameters start, end, top, and height.
 * @param {Object} a          The first Object
 * @param {Object} b          The second Object
 * @return {boolean}        true if a and b collide, else false
 */
export function collisionByTimes(a, b) {

  const timeOverlap = a.start < b.end && a.end > b.start;
  const heightOverlap = a.top < (b.top + b.height) && (a.top + a.height) > b.top;
  return timeOverlap && heightOverlap;

  // return (a.start <= b.start && a.end >= b.start && a.top < (b.top + b.height) && (a.top + a.height) > b.top ) ||
  // (b.start <= a.start && b.end >= a.start && b.top < (a.top + a.height) && (b.top + b.height) > a.top );
}

And it resolved my issue. It looks like this method is only used to layout subgroup heights so it seems pretty safe to modify. I attached an updated, slightly simplified test case that shows the issue when the existing logic is used and shows the proper behavior when the modified logic is used.

I don't know this code too well so I'd appreciate a second set of eyes on the change. I can submit a pull ticket if it makes sense.

I also don't understand why the item top is calculated using half the vertical margin in code such as:

items[i].top = subgroups[items[i].data.subgroup].top + 0.5 * margin.item.vertical;

If you mix normal items and subgroups in the same group the heights don't align properly. But that is another ticket for another day.

expectedVsActualTimesItems.html.zip

@yotamberk
Copy link
Member

Hi @mpilone , PRs are always welcome! Submit one and I will look over it.
It seems that you hit the nail on the head. It should solve the issue as you expect and see.

I also don't understand why the item top is calculated using half the vertical margin in code such as...

I'm honestly not sure... I think the margin was thought of as a margin from top and bottom. So maybe that's why it's half? really not sure... It seems to be wrong to me.

@yotamberk yotamberk added the bug Something isn't working label Mar 7, 2020
mpilone added a commit to mpilone/vis-timeline that referenced this issue Mar 9, 2020
@vis-bot
Copy link
Collaborator

vis-bot commented Mar 9, 2020

🎉 This issue has been resolved in version 7.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants