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
Timeline.add(data) will cause 0 distance between ticks when the timeline is hidden when .add() is called #801
Comments
Timeline typically does not do well when manipulated without being visible onscreen, because it uses screen dimensions to compute all sorts of details about its own layout. This comes up more frequently when people initialize a timeline on a hidden tab, but it seems to be the same problem. Our recommended approach in these cases is to add a call to
That said, I think I get the gist. Trying to sort through your code is making me regret some of the code factoring decisions in the original TimelineJS. Still, this may not be the time to change it. Here are a couple of adjustments I'd like to suggest, in the spirit of nudging this old code towards some better practices:
It would also be great to add a test case so we can more easily check for regressions. I'll be the first to admit we don't have a very robust UI testing system, but perhaps we can discuss ideas. The simplest would probably be to add another NPM script like Does this make sense? PS upon further consideration, I realize that the child elements may need their own container widths instead of something which can be passed in from the containing timeline object... but for right now I don't have time to sort through the code details more .... |
I accidentally posted this while I was still formatting the issue, It should be a little less stream of consciousness now (hopefully). But it sounds like you understand what I was trying to describe.
Based on this, I do think I have over-complicated what I suggested as the fix. I have just checked and replacing
with
This will default the width value to the timeline calculated value from the last time updateDisplay was invoked. |
Regarding Tests. I've looked at the compare.html, and index.html thats being tested via disttest and compare, and im not familiar with how they are setting up and testing the UI elements. I'd be better able to write functional tests around the public functions i'd be making related changes to. |
When an embedded timeline is rendered but is currently hidden, then when when Timeline.add is called, the distance between timeMakers is set to 0 as the current width of the hidden Timeline is calculated as 0 and therefore the distance between each marker is 0.
The Tab the timeline embed is rendered in
Then navigating to a different tab (Add a memory) hides the timeline, and it is while the timeline is hidden that Timeline.add is called and the width is calculated to be zero.
so when navigating back to the Timeline the markers are no longer visible as the distance between them is now 0
Timeline.js
Calls this._timenav._updateDrawTimeline(false);
TimeNav.js
Which then calls this.timeaxis.positionTicks(this.timescale, this.options.optimal_tick_width);
TimeAxis.js
The main issue is that
this._el.container.offsetWidth can be equal to 0 when the timeline is not currently on screen. Which causes the downstream calculation in the TimeAxis to have a display_width of 0 and end up calculating for timescale.getPosition incorrectly
As the display_width is used in the Timescale.js constructor, it will create a _pixel_width of 0
which is what _pixels_per_milli is calculated from, and will also be 0
As updates call the this.timeaxis.positionTicks for getting the Ticks positions, this will always be 0 when _pixels_per_milli as display_width is 0
I suggest that TimeNav.js be updated in 2 locations when it creates a new TimeScale in _getTimeScale(), and in_updateDrawTimeline(check_update)
Changing their respective display_width to either be display_width: this._el.container.offsetWidth, or if that is 0 then default to the this.options.width value so the last known value of the width is then used.
so replace
with
Eg, below is are the functions with this change. I can confirm that this change works for me when implemented for my use case
TimeNav.js
TimeNav.js
Please let me know if theres anything thats unclear, or if you see any concerns with what im proposing
If you are OK with the proposed change I can submit a PR created that includes these changes
The text was updated successfully, but these errors were encountered: