Add a vertical scroll option for timeline [solves #273, #1060, #466] #2196
Conversation
yotamberk
changed the title
mojoaxel
left a comment
mojoaxel
left a comment
There was a problem hiding this comment.
@yotamberk I like that you fixed the RTL support!
But I also have requested quite some changes. I'm sorry! I realy don't want to discourage you but give you constructive feedback. If you have questions don't hesitate to ask me.
| <br> | ||
|
|
||
| <script> | ||
| function showVisibleItems() { |
There was a problem hiding this comment.
looks like this function is never called. I think it should be removed.
| document.getElementById("visibleItemsContainer").innerHTML += a; | ||
| }; | ||
|
|
||
| // get selected item count from url parameter |
There was a problem hiding this comment.
As far as I can see it there are no url parameters.
| var count = 1000; | ||
|
|
||
| // create groups | ||
| var groups = new vis.DataSet([ |
There was a problem hiding this comment.
This could be easily done in a loop, but ok - I don't mind.
| // create items | ||
| var items = new vis.DataSet(); | ||
|
|
||
| var order = 1; |
There was a problem hiding this comment.
Better use order and truck as the loop variables, than you don't need jand i.
|
|
||
| // create a Timeline | ||
| var container = document.getElementById('mytimeline'); | ||
| timeline = new vis.Timeline(container, null, options); |
There was a problem hiding this comment.
ok, this one is mainly just taste but it would be shorter to write:
timeline = new vis.Timeline(container, items, groups, options);I expect that also to be faster, because every setGroup and setItems propably requires a rerendering.
lib/timeline/Core.js
Outdated
|
|
||
| var scrollbarWidth = 0; | ||
| if (this.options.verticalScroll) { | ||
| scrollbarWidth = 16; |
There was a problem hiding this comment.
Sadly the width of the scrollbar can be different in each browser. You need a hack to get it. Here is a nice explanation: http://stackoverflow.com/a/13382873/722162
| <link href="../../../dist/vis.css" rel="stylesheet" type="text/css" /> | ||
|
|
||
|
|
||
| <style type="text/css"> |
There was a problem hiding this comment.
I'm not sure haw other examples handle this, but I think we don't need this style here. The reason is to keep the examples as clean an copyable as possible.
| <script src="../../googleAnalytics.js"></script> | ||
| </head> | ||
|
|
||
| <body onresize="/*timeline.checkResize();*/"> |
There was a problem hiding this comment.
You should remove the onresize.
| <h1>Timeline vertical scroll option</h1> | ||
|
|
||
| <div id="mytimeline"></div> | ||
| <br> |
| }; | ||
|
|
||
| // get selected item count from url parameter | ||
| var count = 1000; |
There was a problem hiding this comment.
It looks like countis only used once. I think the variable is not needed.
|
@yotamberk I also have the strange behavior, that the scrollbar shrinks if i scroll down. |
|
No problem! I'm fixing the other PR and then will work on these changes. No need to apologize! Reviews are necessary and I myself require them to improve! |
|
@mojoaxel I don't see the shrinking problem. Can you elaborate? |
Here you can see how the left scrollbar gets smaller and smaller as I scroll down: |
|
ah... okay that's weird... I'll take a look at it. |
|
@yotamberk And also please resolve the new conflict. Thx! |
|
@yotamberk That is really a big improvement! 🎉 |
|
My pleasure! Working on some other cool features!
|
2 participants
fixes #273, #1060, #466