-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Automargin fixes #2681
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
Merged
Merged
Automargin fixes #2681
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
970db80
stop mutating user data to add uids
alexcjohnson 26278de
some test cleanup
alexcjohnson 6dd68bd
first cut at updating automargin logic
alexcjohnson fa86147
refactor colorbars for simpler connection to trace attributes
alexcjohnson 95f9b33
test legend, rangeselector, rangeslider automargin updates
alexcjohnson 8f3da82
provide uids to traces a little earlier - to fix carpet clip ids
alexcjohnson f9fbbc6
:hocho: colorbar.vals - seems like that was all obsolete anyway
alexcjohnson 222f396
cleanup in sliders_test
alexcjohnson bd07df7
simplify inner plot size tests with custom_assertions
alexcjohnson 84a5695
fix and test slider x positioning when they push margins
alexcjohnson 3deab4c
move sliders to the updated automargin framework
alexcjohnson bb6c720
use bg rect instead of grids for assertPlotSize
alexcjohnson d02c76b
base fractional colorbar size on post-expanded-margin plot size
alexcjohnson 5d31324
comment and fix on assertPlotSize with margin.pad
alexcjohnson 16ea22a
reduce automargin padding for small explicit margins
alexcjohnson 24aeb7c
axRange edits don't need drawMarginPushers after all
alexcjohnson 230a782
automargin comment updates
alexcjohnson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
/** | ||
* Copyright 2012-2018, Plotly, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
|
||
'use strict'; | ||
|
||
var Colorscale = require('../colorscale'); | ||
var drawColorbar = require('./draw'); | ||
|
||
/** | ||
* connectColorbar: create a colorbar from a trace, using its module to | ||
* describe the connection. | ||
* | ||
* @param {DOM element} gd | ||
* | ||
* @param {Array} cd | ||
* calcdata entry for this trace. cd[0].trace is the trace itself, and the | ||
* colorbar object will be stashed in cd[0].t.cb | ||
* | ||
* @param {object|function} moduleOpts | ||
* may be a function(gd, cd) to override the standard handling below. If | ||
* an object, should have these keys: | ||
* @param {Optional(string)} moduleOpts.container | ||
* name of the container inside the trace where the colorbar and colorscale | ||
* attributes live (ie 'marker', 'line') - omit if they're at the trace root. | ||
* @param {string} moduleOpts.min | ||
* name of the attribute holding the value of the minimum color | ||
* @param {string} moduleOpts.max | ||
* name of the attribute holding the value of the maximum color | ||
* @param {Optional(string)} moduleOpts.vals | ||
* name of the attribute holding the (numeric) color data | ||
* used only if min/max fail. May be omitted if these are always | ||
* pre-calculated. | ||
*/ | ||
module.exports = function connectColorbar(gd, cd, moduleOpts) { | ||
if(typeof moduleOpts === 'function') return moduleOpts(gd, cd); | ||
|
||
var trace = cd[0].trace; | ||
var cbId = 'cb' + trace.uid; | ||
var containerName = moduleOpts.container; | ||
var container = containerName ? trace[containerName] : trace; | ||
|
||
gd._fullLayout._infolayer.selectAll('.' + cbId).remove(); | ||
if(!container || !container.showscale) return; | ||
|
||
var zmin = container[moduleOpts.min]; | ||
var zmax = container[moduleOpts.max]; | ||
|
||
var cb = cd[0].t.cb = drawColorbar(gd, cbId); | ||
var sclFunc = Colorscale.makeColorScaleFunc( | ||
Colorscale.extractScale( | ||
container.colorscale, | ||
zmin, | ||
zmax | ||
), | ||
{ noNumericCheck: true } | ||
); | ||
|
||
cb.fillcolor(sclFunc) | ||
.filllevels({start: zmin, end: zmax, size: (zmax - zmin) / 254}) | ||
.options(container.colorbar)(); | ||
}; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,15 +61,9 @@ module.exports = function(gd) { | |
|
||
// remove exiting sliders and their corresponding clip paths | ||
rangeSliders.exit().each(function(axisOpts) { | ||
var rangeSlider = d3.select(this), | ||
opts = axisOpts[constants.name]; | ||
|
||
rangeSlider.remove(); | ||
var opts = axisOpts[constants.name]; | ||
fullLayout._topdefs.select('#' + opts._clipId).remove(); | ||
}); | ||
|
||
// remove push margin object(s) | ||
if(rangeSliders.exit().size()) clearPushMargins(gd); | ||
}).remove(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha |
||
|
||
// return early if no range slider is visible | ||
if(rangeSliderData.length === 0) return; | ||
|
@@ -602,16 +596,3 @@ function drawGrabbers(rangeSlider, gd, axisOpts, opts) { | |
}); | ||
grabAreaMax.attr('height', opts._height); | ||
} | ||
|
||
function clearPushMargins(gd) { | ||
var pushMargins = gd._fullLayout._pushmargin || {}, | ||
keys = Object.keys(pushMargins); | ||
|
||
for(var i = 0; i < keys.length; i++) { | ||
var k = keys[i]; | ||
|
||
if(k.indexOf(constants.name) !== -1) { | ||
Plots.autoMargin(gd, k); | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, contour still gets its own version on
module._colorbar
inhttps://github.com/plotly/plotly.js/blob/fa8614750586a2901ccf65a520521bb570eb7026/src/traces/contour/colorbar.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and that's the only exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, contour and those that inherit from it (
contourcarpet
,contourgl
,histogram2dcontour
).I should note that there's one other place
module.colorbar
gets used, that's incolorbar.draw
for getting thenestedProperty
string for title editing. If contour colorbars weren't at the module root we'd need to set acontainer
property of the function which would be a bit hacky... alternatively we could either a) make this function bemodule.colorbar.func
, or b) incorporate contour into this routine directly, triggered by some other property ofmodule.colorbar
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds cleaner to me. But no need to do this in this PR.