Skip to content

traffic_graphs.widget: start when widgets is visible#4801

Merged
AdSchellevis merged 3 commits intoopnsense:masterfrom
kulikov-a:widg-event
Mar 11, 2021
Merged

traffic_graphs.widget: start when widgets is visible#4801
AdSchellevis merged 3 commits intoopnsense:masterfrom
kulikov-a:widg-event

Conversation

@kulikov-a
Copy link
Copy Markdown
Member

@kulikov-a kulikov-a commented Mar 11, 2021

Hi!
after a long investigation in the "messages" on the forum, it turned out that the traffic_graph widget is still not quite stable in the Firefox browser. The FF can start requests before the widgets are visible. As a result, the chart plugin assigns the canvas height to 0 (chartjs does not like if the parent element is hidden when the chart is initialized) and it start to look like #4750.
To reproduce the behavior, you can set a breakpoint on the $("#traffic_graphs-configure").removeClass("disabled"); in FF and refresh the page. It should be seen that the widgets div are not yet visible before executing the request.
Could you consider adding an event after placing widgets and launching a widget functions on this trigger?
This change tested and gave: "TF loads every time on both my desktop (with all extensions enabled) and mobile FF!"
Thanks!
and that trigger can be used on other client-side widgets to start fetching when whole page is set
ps.
and its looks like this lines not needed any more. since we cut scripts from widgets. works with or without it

// XXX: since dashboard widgets may have changed the dom, prevent on load being executed multiple times.
// it's not very pretty, but prevents mangled graphs
if (window.traffic_graph_widget_loaded !== undefined) {
return;
}
window.traffic_graph_widget_loaded = true;

@AdSchellevis
Copy link
Copy Markdown
Member

you're right, we can drop the traffic_graph_widget_loaded as well now, thanks! if you update the PR, I'll make sure to pull both in.

@kulikov-a
Copy link
Copy Markdown
Member Author

@AdSchellevis thanks!! updated

@AdSchellevis AdSchellevis merged commit 3274620 into opnsense:master Mar 11, 2021
@AdSchellevis
Copy link
Copy Markdown
Member

@kulikov-a looks good, thanks! I haven't tried it myself yet, but it looks safe enough to pull in now

@kulikov-a
Copy link
Copy Markdown
Member Author

@AdSchellevis thanks again!

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants