Move visualiser graph type default to a constant #2138
Conversation
In my fork of Luigi, I use svg as the graph type default. This results in annoying merges when javascript related to handling the defaults changes. In order to make different defaults easier to maintain, this uses a constant in visualiserApp.js to determine which graph type is default rather than hard-coding it everywhere the default value is used. This also cleans up a few places where the default was being set unnecessarily: - don't set it in html since we set it in javascript instantly anyway - remove code to set buttons to checked since this has no meaning
|
@daveFNbuck, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nmb10, @riga and @Tarrasch to be potential reviewers. |
Before the previous commit, the graph type default was actually svg, despite the d3 checkbox being checked by default. In order to keep more consistent behavior, we should set the default graph type to svg.
|
While working on #2139 I discovered that the current default graph type is actually svg, despite code that attempts to make it d3. It seems we default the radio buttons to d3 but the graph behavior to svg. To minimize the effects of this change on the end user, I've added a commit that changes the default to svg. These commits now combine to fix a bug where by default you have an svg graph showing under a clicked d3 button, and you can't click the already clicked d3 button to change it. |
|
Note: this failed travis due to a timeout. Nothing to do with this PR as it has no Python code changes. |
|
Finally a red diff! :) |
|
Hopefully I'll be done with the javascript stuff soon and I can get back to red-heavy Python diffs :) |
Description
Refactors the way the visType default is handled so it's controlled by a constant defined at the top of visualiserApp.js
This also cleans up a few places where the default was being set
unnecessarily:
Motivation and Context
In my fork of Luigi, I use svg as the graph type default. This results in annoying merges when javascript related to handling the defaults changes. In order to make different defaults easier to maintain, this uses a constant in visualiserApp.js to determine which graph type is default rather than hard-coding it everywhere the default value is used.
Have you tested this? If so, how?
I ran it locally with both d3 and svg as defaults and tried loading pages. It seems to work.