NUKnightLab / NUKnightLab/TimelineJS3

Timeline.add(data) will cause 0 distance between ticks when the timeline is hidden when .add() is called

Open
#801 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
3.2k
Forks
645
PR merge metrics
No merged PRs in 30d

Description

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
image

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
image

Timeline.js

// Add an event
add(data) {
    var unique_id = this.config.addEvent(data);

    var n = this._getEventIndex(unique_id);
    var d = this.config.events[n];

    this._storyslider.createSlide(d, this.config.title ? n + 1 : n);
    this._storyslider._updateDrawSlides();

    this._timenav.createMarker(d, n);
    this._timenav._updateDrawTimeline(false);

    this.fire("added", { unique_id: unique_id });
}

Calls this._timenav._updateDrawTimeline(false);

TimeNav.js

_updateDrawTimeline(check_update) {
    var do_update = false;

    // Check to see if redraw is needed
    if (check_update) {
        /* keep this aligned with _getTimeScale or reduce code duplication */
        var temp_timescale = new TimeScale(this.config, {
            display_width: this._el.container.offsetWidth,
            screen_multiplier: this.options.scale_factor,
            max_rows: this.max_rows

        });

        if (this.timescale.getMajorScale() == temp_timescale.getMajorScale() &&
            this.timescale.getMinorScale() == temp_timescale.getMinorScale()) {
            do_update = true;
        }
    } else {
        do_update = true;
    }

    // Perform update or redraw
    if (do_update) {
        this.timescale = this._getTimeScale();
        this.timeaxis.positionTicks(this.timescale, this.options.optimal_tick_width);
        this.positionMarkers();
        this._assignRowsToMarkers();
        this._positionGroups();
        if (this.has_eras) {
            this._positionEras();
        }
        this.updateDisplay();
    } else {
        this._drawTimeline(true);
    }

    return do_update;

}

Which then calls this.timeaxis.positionTicks(this.timescale, this.options.optimal_tick_width);

TimeAxis.js

_positionTickArray(tick_array, timescale, optimal_tick_width) {
    // Poition Ticks & Handle density of ticks
    if (tick_array[1] && tick_array[0]) {
        var distance = (timescale.getPosition(tick_array[1].date.getMillisecond()) - timescale.getPosition(tick_array[0].date.getMillisecond())),
            fraction_of_array = 1;


        if (distance < optimal_tick_width) {
            fraction_of_array = Math.round(optimal_tick_width / timescale.getPixelsPerTick());
        }

        var show = 1;

        for (var i = 0; i < tick_array.length; i++) {

            var tick = tick_array[i];

            // Poition Ticks
            tick.tick.style.left = timescale.getPosition(tick.date.getMillisecond()) + "px";
            tick.tick_text.innerHTML = tick.display_date;

            // Handle density of ticks
            if (fraction_of_array > 1) {
                if (show >= fraction_of_array) {
                    show = 1;
                    tick.tick_text.style.opacity = 1;
                    tick.tick.className = "tl-timeaxis-tick";
                } else {
                    show++;
                    tick.tick_text.style.opacity = 0;
                    tick.tick.className = "tl-timeaxis-tick tl-timeaxis-tick-hidden";
                }
            } else {
                tick.tick_text.style.opacity = 1;
                tick.tick.className = "tl-timeaxis-tick";
            }

        }
    }
}

The main issue is that

display_width: this._el.container.offsetWidth,

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

tick.tick.style.left = timescale.getPosition(tick.date.getMillisecond()) + "px";

As the display_width is used in the Timescale.js constructor, it will create a _pixel_width of 0

this._pixel_width = this._screen_multiplier * this._display_width;

which is what _pixels_per_milli is calculated from, and will also be 0

 this._pixels_per_milli = this.getPixelWidth() / this._span_in_millis;

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

getPosition(time_in_millis) {
    // be careful using millis, as they won't scale to cosmological time.
    // however, we're moving to make the arg to this whatever value
    // comes from TLDate.getTime() which could be made smart about that --
    // so it may just be about the naming.
    return (time_in_millis - this._earliest) * this._pixels_per_milli
}

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

display_width: this._el.container.offsetWidth,

with

display_width: this._el.container.offsetWidth === 0 ? this.options.width : this._el.container.offsetWidth,

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

_getTimeScale() {
    /* maybe the establishing config values (marker_height_min and max_rows) should be
    separated from making a TimeScale object, which happens in another spot in this file with duplicate mapping of properties of this TimeNav into the TimeScale options object? */
    // Set Max Rows
    var marker_height_min = 0;
    try {
        marker_height_min = parseInt(this.options.marker_height_min);
    } catch (e) {
        trace("Invalid value for marker_height_min option.");
        marker_height_min = 30;
    }
    if (marker_height_min == 0) {
        trace("marker_height_min option must not be zero.")
        marker_height_min = 30;
    }
    this.max_rows = Math.round((this.options.height - this._el.timeaxis_background.offsetHeight - (this.options.marker_padding)) / marker_height_min);
    if (this.max_rows < 1) {
        this.max_rows = 1;
    }
    return new TimeScale(this.config, {
        display_width: this._el.container.offsetWidth === 0 ? this.options.width : this._el.container.offsetWidth,
        screen_multiplier: this.options.scale_factor,
        max_rows: this.max_rows

    });
}

TimeNav.js

_updateDrawTimeline(check_update) {
    var do_update = false;

    // Check to see if redraw is needed
    if (check_update) {
        /* keep this aligned with _getTimeScale or reduce code duplication */
        var temp_timescale = new TimeScale(this.config, {
            display_width: this._el.container.offsetWidth === 0 ? this.options.width : this._el.container.offsetWidth,
            screen_multiplier: this.options.scale_factor,
            max_rows: this.max_rows

        });

        if (this.timescale.getMajorScale() == temp_timescale.getMajorScale() &&
            this.timescale.getMinorScale() == temp_timescale.getMinorScale()) {
            do_update = true;
        }
    } else {
        do_update = true;
    }

    // Perform update or redraw
    if (do_update) {
        this.timescale = this._getTimeScale();
        this.timeaxis.positionTicks(this.timescale, this.options.optimal_tick_width);
        this.positionMarkers();
        this._assignRowsToMarkers();
        this._positionGroups();
        if (this.has_eras) {
            this._positionEras();
        }
        this.updateDisplay();
    } else {
        this._drawTimeline(true);
    }

    return do_update;

}

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in TimeNav.js with _getTimeScale() and _updateDrawTimeline(), then trace the display_width value into TimeScale.js and TimeAxis.js. Reproduce Timeline.add() while the timeline container is hidden, and verify that markers and ticks retain usable spacing when the timeline becomes visible again.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
frontend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.