8000 Fix setting tickwidth, tickcolor, ticklen, linecolor and possibly more attributes via template by archmoj · Pull Request #4904 · plotly/plotly.js · GitHub
[go: up one dir, main page]
Skip to content

Fix setting tickwidth, tickcolor, ticklen, linecolor and possibly more attributes via template #4904

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Jun 17, 2020
Merged

Fix setting tickwidth, tickcolor, ticklen, linecolor and possibly more attributes via template #4904

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bddaafa
allow template input in Lib.coerce2 - fix issue 4852
archmoj Jun 5, 2020
11fd23c
update axes_breaks-round-weekdaysm baseline
archmoj Jun 5, 2020
756a0ee
refactor coerce rename v to valIn
archmoj Jun 7, 2020
a2c1c93
refactor coerce rename out to valOut
archmoj Jun 7, 2020
8d0b4cb
exit early
archmoj Jun 7, 2020
5c3be81
rename propOut to valOut
archmoj Jun 7, 2020
642bf28
add internal _coerce function
archmoj Jun 7, 2020
57cec14
reduce condition checks
archmoj Jun 8, 2020
1d9cc4a
keep track of source and input in _coerce function
archmoj Jun 9, 2020
cf7c061
fixup Lib.coerce2 description and color test
archmoj Jun 10, 2020
21b86cd
add jasmine tests to cover invalid inputs in Lib.coerce2
archmoj Jun 10, 2020
2b911d1
make room for opts - rename opts to attr
archmoj Jun 16, 2020
adc36cd
validate input when calling from coerce2
archmoj Jun 16, 2020
24d1da9
remove unnecessary validation, also
archmoj Jun 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
rename propOut to valOut
  • Loading branch information
@archmoj
archmoj committed Jun 7, 2020
commit 5c3be81ee1073d199b8399ff02ce8dd7ddf0a3fa
8 changes: 4 additions & 4 deletions src/lib/coerce.js
8000
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,19 +408,19 @@ exports.coerce = function(containerIn, containerOut, attributes, attribute, dflt
* returns false if there is no user input.
*/
exports.coerce2 = function(containerIn, containerOut, attributes, attribute, dflt) {
var propOut = exports.coerce(containerIn, containerOut, attributes, attribute, dflt);
var valOut = exports.coerce(containerIn, containerOut, attributes, attribute, dflt);

var attr = attributes[attribute];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

    var attr = nestedProperty(attributes, attribute).get();

From coerce (line 364 above) - then the attr || {} below would be unnecessary - attr will always exist.

But I'm a little concerned whether the logic below (theDefault !== undefined && theDefault !== propOut) is robust - what if your template explicitly provides the same value as the default? If valIn does that, we return propOut, but given the logic here if the template does that we'll return false.

Could we instead make a base function like _coerce that returns both the value and where it came from (default, template, or container), then have exports.coerce just ignore that second part and return the value, whereas exports.coerce2 returns false if the value came from a default, otherwise the value?

Actually isn't there another already existing problem with coerce2? Looks like if you provide an invalid valIn, the prop will be set to the default value but it'll look like you set it explicitly. The base function approach would fix that problem too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcjohnson thanks for the review.
Please find the commits below.

var theDefault = (dflt !== undefined) ? dflt : (attr || {}).dflt;
if(
theDefault !== undefined &&
theDefault !== propOut
theDefault !== valOut
) {
return propOut;
return valOut;
}

var valIn = nestedProperty(containerIn, attribute).get();
return (valIn !== undefined && valIn !== null) ? propOut : false;
return (valIn !== undefined && valIn !== null) ? valOut : false;
};

/*
Expand Down
0