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

[bug] Line: flow() breaks (drag-)Zoom #2140

Closed
adschm opened this issue Jun 14, 2021 · 8 comments
Closed

[bug] Line: flow() breaks (drag-)Zoom #2140

adschm opened this issue Jun 14, 2021 · 8 comments

Comments

@adschm
Copy link
Contributor

adschm commented Jun 14, 2021

I show historic timeseries data and then add "live" data points by using .flow() function to append the latest datapoint to that view. As soon as .flow() is executed the first time, drag-zoom is broken. (Before the first trigger is executed, everything works fine.)

Sometimes it's starting to work again after some time or a few tries, but this is not reproducible (whereas the broken zoom is).

A simple example is found here:
https://jsfiddle.net/adschm/2t0ao5h4/18/

Just try to drag-zoom. Setting refresh variable to false will produce a zoomable plot.

Chromium-based console gives me the following (transition id varies) for each unsuccessful click:

Uncaught Error: transition 837 not found
    at vo (d3.v6.min.js:2)
    at Pn.zn.transition (d3.v6.min.js:2)
    at AxisRendererHelper.transitionise (billboard.custom.js:9434)
    at SVGGElement.<anonymous> (billboard.custom.js:9519)
    at ji.each (d3.v6.min.js:2)
    at AxisRenderer.create (billboard.custom.js:9510)
    at billboard.custom.js:10216
    at Array.forEach (<anonymous>)
    at Axis.redraw (billboard.custom.js:10213)
    at Axis.redrawAxis (billboard.custom.js:10253)

vo	@	d3.v6.min.js:2
zn.transition	@	d3.v6.min.js:2
transitionise	@	billboard.custom.js:9434
(anonymous)	@	billboard.custom.js:9519
each	@	d3.v6.min.js:2
create	@	billboard.custom.js:9510
(anonymous)	@	billboard.custom.js:10216
redraw	@	billboard.custom.js:10213
redrawAxis	@	billboard.custom.js:10253
redraw	@	billboard.custom.js:5382
zoom	@	billboard.custom.js:16477
(anonymous)	@	billboard.custom.js:17306
call	@	d3.v6.min.js:2
e	@	d3.v6.min.js:2
g	@	d3.v6.min.js:2
(anonymous)	@	d3.v6.min.js:2
@netil netil added the bug label Jun 15, 2021
@netil
Copy link
Member

netil commented Jun 15, 2021

Hi @adschm, thanks for the report. I'll check the issue.

@netil netil self-assigned this Jun 15, 2021
@adschm
Copy link
Contributor Author

adschm commented Jun 15, 2021

BTW: Is there a simpler function available to "just append data" (to an existing series)? I only use .flow() here because I didn't find anything else, but I do not need all the additional animation handles ...

@netil
Copy link
Member

netil commented Jun 15, 2021

@adschm, to simply load data dynamically, use .load() instead of .flow().

@adschm
Copy link
Contributor Author

adschm commented Jun 15, 2021

The way I understand .load() is that it replaces data when I specify an already existing data id?

My problem is that my first DB query for the data is typically expensive, so I don't want to re-query or concatenate data more often than necessary.

How would I change my referenced example to achieve the same result by using load?

@netil
Copy link
Member

netil commented Jun 16, 2021

@adschm for dynamic data loading, checkout the wiki doc.

And for appending data, you need handle data to be displayed separately.

// 1) initial data
var data = [
      ["x", '2021-01-01T08:00:00', '2021-01-01T15:00:00', '2021-01-02T12:00:00', '2021-01-02T23:00:00', '2021-01-03T03:00:00', '2021-01-04T12:00:00', '2021-01-05T21:00:00'],
      ["sample2", 12, 64, 23, 75, 36, 30, 24]
    ];

var chart = bb.generate({
    data: {
        columns: data
    }, 
    ...
};

// 2) when need to append data, just handle it as your needs
data[0].push('2021-02-01T08:00:00');
data[1].push(37);

// 3) then, use updated data
chart.load({columns: data});    

@netil netil added question and removed bug labels Jun 16, 2021
netil added a commit to netil/billboard.js that referenced this issue Jun 17, 2021
Implement data append option for .load() API

Ref naver#2140
@netil netil closed this as completed in 8076795 Jun 17, 2021
@adschm
Copy link
Contributor Author

adschm commented Jun 18, 2021

@netil Thanks for packaging append, it seems to work from a quick test and resolves my specific use-case.

However, does this actually fix the reported issue, i.e. zoom broken after flow()? If drag-zoom does not make sense during flow(), it should be disabled in this case. Otherwise, it should be fixed properly, IMO.
Probably, this should be reopened until flow() + zoom is dealt with.

github-actions bot pushed a commit that referenced this issue Jun 25, 2021
# [3.1.0](3.0.3...3.1.0) (2021-06-25)

### Bug Fixes

* **axis:** Fix axis.x.padding value setting ([5b4b509](5b4b509)), closes [#2038](#2038)
* **axis:** fix handling x padding value ([489d47a](489d47a)), closes [#2038](#2038)
* **candlestick:** fix to set expand state ([a055b20](a055b20)), closes [#2036](#2036)
* **Chart:** Handle nullish properties from API extendings safely ([6cbf64a](6cbf64a)), closes [#2132](#2132) [#2134](#2134)
* **data:** Fix duplicated data.onclick call  ([b4c5dc2](b4c5dc2)), closes [#2104](#2104)
* **data:** Fix nullish data filtering for grouped data ([af19370](af19370)), closes [#2096](#2096)
* **gauge:** Fix incorrect rendering when gauge.min is given ([31fc981](31fc981)), closes [#2123](#2123)
* **point:** Fix custom point for nullish data ([8c198f2](8c198f2)), closes [#2107](#2107)
* **region:** fix region append position ([2b50443](2b50443)), closes [#2067](#2067)
* **size:** enhance applying height value ([0664a60](0664a60)), closes [#2086](#2086)
* **tooltip:** Correct the type of selectedData ([05b694d](05b694d)), closes [#2056](#2056) [#2058](#2058)
* **types:** fix missing candlestick export ([f218939](f218939)), closes [#2007](#2007)
* **types:** updated bar/candlestick options types ([d89c3f3](d89c3f3)), closes [#2043](#2043)
* **zoom:** Fix incorrect tooltip position ([689bfdf](689bfdf)), closes [#2095](#2095)
* **zoom,grid:** fix grid line pos during zoom ([e84a4f1](e84a4f1)), closes [#2156](#2156)

### Features

* **all:** contain inline css prop setting ([fde6a89](fde6a89)), closes [#2076](#2076)
* **api:** Intent to ship append load ([8076795](8076795)), closes [#2140](#2140)
* **data:** Intent to ship data.onshown/onhidden ([af98eb7](af98eb7)), closes [#2146](#2146)
* **data.labels:** Intent to ship data.labels.backgroundColors ([e0b2fed](e0b2fed)), closes [#1954](#1954)
* **subchart:** Intent to ship subchart.init.range option ([967bf1b](967bf1b)), closes [#2037](#2037)
* **subchart:** Intent to ship subchart.showHandle ([219bff3](219bff3)), closes [#2044](#2044)
netil pushed a commit that referenced this issue Jun 25, 2021
* **Chart:** Handle nullish properties from API extendings safely ([6cbf64a](6cbf64a)), closes [#2132](#2132) [#2134](#2134)
* **data:** Fix duplicated data.onclick call  ([b4c5dc2](b4c5dc2)), closes [#2104](#2104)
* **data:** Fix nullish data filtering for grouped data ([af19370](af19370)), closes [#2096](#2096)
* **gauge:** Fix incorrect rendering when gauge.min is given ([31fc981](31fc981)), closes [#2123](#2123)
* **point:** Fix custom point for nullish data ([8c198f2](8c198f2)), closes [#2107](#2107)
* **region:** fix region append position ([2b50443](2b50443)), closes [#2067](#2067)
* **size:** enhance applying height value ([0664a60](0664a60)), closes [#2086](#2086)
* **tooltip:** Correct the type of selectedData ([05b694d](05b694d)), closes [#2056](#2056) [#2058](#2058)
* **zoom:** Fix incorrect tooltip position ([689bfdf](689bfdf)), closes [#2095](#2095)
* **zoom,grid:** fix grid line pos during zoom ([e84a4f1](e84a4f1)), closes [#2156](#2156)

* **all:** contain inline css prop setting ([fde6a89](fde6a89)), closes [#2076](#2076)
* **api:** Intent to ship append load ([8076795](8076795)), closes [#2140](#2140)
* **data:** Intent to ship data.onshown/onhidden ([af98eb7](af98eb7)), closes [#2146](#2146)
* **data.labels:** Intent to ship data.labels.backgroundColors ([e0b2fed](e0b2fed)), closes [#1954](#1954)
* **subchart:** Intent to ship subchart.init.range option ([967bf1b](967bf1b)), closes [#2037](#2037)
* **subchart:** Intent to ship subchart.showHandle ([219bff3](219bff3)), closes [#2044](#2044)
@netil
Copy link
Member

netil commented Jun 30, 2021

However, does this actually fix the reported issue, i.e. zoom broken after flow()? If drag-zoom does not make sense during flow(), it should be disabled in this case.

Doing some interaction during the middle of the transition, will eventually make an unexpected behavior or result.
Maybe adding some prevention to not interact is needed.

@netil netil reopened this Jun 30, 2021
netil added a commit to netil/billboard.js that referenced this issue Jun 30, 2021
- Prevent transition selection to throw error
- Make state.redrawing to be set false after redraw calls

Ref naver#2140
@netil netil closed this as completed in f0cbe6b Jun 30, 2021
@netil
Copy link
Member

netil commented Jun 30, 2021

doing drag after the .flow() will not throw error.
Kapture 2021-06-30 at 18 52 20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants