Skip to content

Negate axis offset and then turn it into a string #2339

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 7 commits into from
Feb 9, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 7 additions & 3 deletions src/components/fx/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,16 +175,18 @@ function _hover(gd, evt, subplot, noHoverEvent) {
subplots = subplots.concat(overlayedSubplots);
}

var len = subplots.length,
xaArray = new Array(len),
yaArray = new Array(len);
var len = subplots.length;
var xaArray = new Array(len);
var yaArray = new Array(len);
var supportsCompare = false;

for(var i = 0; i < len; i++) {
var spId = subplots[i];

// 'cartesian' case
var plotObj = plots[spId];
if(plotObj) {
supportsCompare = true;

// TODO make sure that fullLayout_plots axis refs
// get updated properly so that we don't have
Expand All @@ -203,6 +205,8 @@ function _hover(gd, evt, subplot, noHoverEvent) {

var hovermode = evt.hovermode || fullLayout.hovermode;

if(hovermode && !supportsCompare) hovermode = 'closest';
Copy link
Collaborator

Choose a reason for hiding this comment

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

The second part of #2338 - fixes hover labels for geo and ternary on the plot_types mock

Copy link
Contributor Author

@etpinard etpinard Feb 9, 2018

Choose a reason for hiding this comment

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

Looks good. Not sure what's the best to 🔒 this down?

Maybe just checking that hovermode: 'x' behaves the same as hovermode: 'closest' in a cartesian + geo (or ternary or polar) graph would suffice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

good call -> 16b01f0


if(['x', 'y', 'closest'].indexOf(hovermode) === -1 || !gd.calcdata ||
gd.querySelector('.zoombox') || gd._dragging) {
return dragElement.unhoverRaw(gd, evt);
Expand Down
4 changes: 4 additions & 0 deletions src/components/modebar/buttons.js
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,10 @@ function handleDrag3d(gd, ev) {
layoutUpdate[sceneIds[i] + '.' + parts[1]] = val;
}

// for multi-type subplots
var val2d = (val === 'pan') ? val : 'zoom';
layoutUpdate.dragmode = val2d;

Plotly.relayout(gd, layoutUpdate);
}

Expand Down
91 changes: 55 additions & 36 deletions src/components/modebar/manage.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,13 @@ function getButtonGroups(gd, buttonsToRemove, buttonsToAdd) {
var hasTernary = fullLayout._has('ternary');
var hasMapbox = fullLayout._has('mapbox');
var hasPolar = fullLayout._has('polar');
var allAxesFixed = areAllAxesFixed(fullLayout);

var groups = [];

function addGroup(newGroup) {
if(!newGroup.length) return;

var out = [];

for(var i = 0; i < newGroup.length; i++) {
Expand All @@ -100,55 +103,71 @@ function getButtonGroups(gd, buttonsToRemove, buttonsToAdd) {
// buttons common to all plot types
addGroup(['toImage', 'sendDataToCloud']);

// graphs with more than one plot types get 'union buttons'
// which reset the view or toggle hover labels across all subplots.
if((hasCartesian || hasGL2D || hasPie || hasTernary) + hasGeo + hasGL3D > 1) {
addGroup(['resetViews', 'toggleHover']);
return appendButtonsToGroups(groups, buttonsToAdd);
}
var zoomGroup = [];
var hoverGroup = [];
var resetGroup = [];
var dragModeGroup = [];

if(hasGL3D) {
addGroup(['zoom3d', 'pan3d', 'orbitRotation', 'tableRotation']);
addGroup(['resetCameraDefault3d', 'resetCameraLastSave3d']);
addGroup(['hoverClosest3d']);
if((hasCartesian || hasGL2D || hasPie || hasTernary) + hasGeo + hasGL3D + hasMapbox + hasPolar > 1) {
// graphs with more than one plot types get 'union buttons'
// which reset the view or toggle hover labels across all subplots.
hoverGroup = ['toggleHover'];
resetGroup = ['resetViews'];
}
else if(hasGeo) {
zoomGroup = ['zoomInGeo', 'zoomOutGeo'];
hoverGroup = ['hoverClosestGeo'];
resetGroup = ['resetGeo'];
}
else if(hasGL3D) {
hoverGroup = ['hoverClosest3d'];
resetGroup = ['resetCameraDefault3d', 'resetCameraLastSave3d'];
}
else if(hasMapbox) {
hoverGroup = ['toggleHover'];
resetGroup = ['resetViewMapbox'];
}
else if(hasGL2D) {
hoverGroup = ['hoverClosestGl2d'];
}
else if(hasPie) {
hoverGroup = ['hoverClosestPie'];
}
else { // hasPolar, hasTernary
// always show at least one hover icon.
hoverGroup = ['toggleHover'];
}
// if we have cartesian, allow switching between closest and compare
// regardless of what other types are on the plot, since they'll all
// just treat any truthy hovermode as 'closest'
if(hasCartesian) {
hoverGroup = ['toggleSpikelines', 'hoverClosestCartesian', 'hoverCompareCartesian'];
}

var allAxesFixed = areAllAxesFixed(fullLayout),
dragModeGroup = [];
if((hasCartesian || hasGL2D) && !allAxesFixed) {
zoomGroup = ['zoomIn2d', 'zoomOut2d', 'autoScale2d'];
if(resetGroup[0] !== 'resetViews') resetGroup = ['resetScale2d'];
}

if(((hasCartesian || hasGL2D) && !allAxesFixed) || hasTernary) {
if(hasGL3D) {
dragModeGroup = ['zoom3d', 'pan3d', 'orbitRotation', 'tableRotation'];
}
else if(((hasCartesian || hasGL2D) && !allAxesFixed) || hasTernary) {
dragModeGroup = ['zoom2d', 'pan2d'];
}
if(hasMapbox || hasGeo) {
else if(hasMapbox || hasGeo) {
dragModeGroup = ['pan2d'];
}
if(hasPolar) {
else if(hasPolar) {
dragModeGroup = ['zoom2d'];
}
if(isSelectable(fullData)) {
dragModeGroup.push('select2d');
dragModeGroup.push('lasso2d');
}
if(dragModeGroup.length) addGroup(dragModeGroup);

if((hasCartesian || hasGL2D) && !allAxesFixed && !hasTernary) {
addGroup(['zoomIn2d', 'zoomOut2d', 'autoScale2d', 'resetScale2d']);
dragModeGroup.push('select2d', 'lasso2d');
}

if(hasCartesian && hasPie) {
addGroup(['toggleHover']);
} else if(hasGL2D) {
addGroup(['hoverClosestGl2d']);
} else if(hasCartesian) {
addGroup(['toggleSpikelines', 'hoverClosestCartesian', 'hoverCompareCartesian']);
} else if(hasPie) {
addGroup(['hoverClosestPie']);
} else if(hasMapbox) {
addGroup(['resetViewMapbox', 'toggleHover']);
} else if(hasGeo) {
addGroup(['zoomInGeo', 'zoomOutGeo', 'resetGeo']);
addGroup(['hoverClosestGeo']);
}
addGroup(dragModeGroup);
addGroup(zoomGroup.concat(resetGroup));
addGroup(hoverGroup);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice cleanup. Looking good!


return appendButtonsToGroups(groups, buttonsToAdd);
}
Expand Down
4 changes: 2 additions & 2 deletions src/plots/ternary/ternary.js
Original file line number Diff line number Diff line change
Expand Up @@ -328,12 +328,12 @@ proto.adjustLayout = function(ternaryLayout, graphSize) {
_this.layers.bgrid.attr('transform', bTransform);

var aTransform = 'translate(' + (x0 + w / 2) + ',' + y0 +
')rotate(30)translate(0,-' + aaxis._offset + ')';
')rotate(30)translate(0,' + -aaxis._offset + ')';
Copy link
Contributor Author

@etpinard etpinard Feb 6, 2018

Choose a reason for hiding this comment

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

When _offset is negative we use to get:

image

that is, a double - in the translate transform string.

Looks like [email protected] doesn't care, so this went unnoticed in our image tests.

_this.layers.aaxis.attr('transform', aTransform);
_this.layers.agrid.attr('transform', aTransform);

var cTransform = 'translate(' + (x0 + w / 2) + ',' + y0 +
')rotate(-30)translate(0,-' + caxis._offset + ')';
')rotate(-30)translate(0,' + -caxis._offset + ')';
_this.layers.caxis.attr('transform', cTransform);
_this.layers.cgrid.attr('transform', cTransform);

Expand Down
8 changes: 7 additions & 1 deletion test/image/strict-d3.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,18 @@ selProto.style = function() {
return originalSelStyle.apply(sel, arguments);
};

function checkAttrVal(sel, key) {
function checkAttrVal(sel, key, val) {
// setting the transform attribute on a <clipPath> does not
// work in Chrome, IE and Edge
if(sel.node().nodeName === 'clipPath' && key === 'transform') {
throw new Error('d3 selection.attr called with key \'transform\' on a clipPath node');
}

// make sure no double-negative string get into the DOM,
// their handling differs from browsers to browsers
if(/--/.test(val) && isNumeric(val.split('--')[1].charAt(0))) {
throw new Error('d3 selection.attr called with value ' + val + ' which includes a double negative');
}
}

function checkStyleVal(sel, key, val) {
Expand Down
8 changes: 4 additions & 4 deletions test/jasmine/tests/gl3d_plot_interact_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ describe('@gl Test gl3d modebar handlers', function() {

buttonZoom3d.click();
assertScenes(gd._fullLayout, 'dragmode', 'zoom');
expect(gd.layout.dragmode).toBe(undefined);
expect(gd.layout.dragmode).toBe('zoom'); // for multi-type subplots
expect(gd._fullLayout.dragmode).toBe('zoom');
expect(buttonTurntable.isActive()).toBe(false);
expect(buttonZoom3d.isActive()).toBe(true);
Expand All @@ -504,8 +504,8 @@ describe('@gl Test gl3d modebar handlers', function() {

buttonPan3d.click();
assertScenes(gd._fullLayout, 'dragmode', 'pan');
expect(gd.layout.dragmode).toBe(undefined);
expect(gd._fullLayout.dragmode).toBe('zoom');
expect(gd.layout.dragmode).toBe('pan'); // for multi-type subplots
expect(gd._fullLayout.dragmode).toBe('pan');
expect(buttonTurntable.isActive()).toBe(false);
expect(buttonPan3d.isActive()).toBe(true);

Expand All @@ -525,7 +525,7 @@ describe('@gl Test gl3d modebar handlers', function() {

buttonOrbit.click();
assertScenes(gd._fullLayout, 'dragmode', 'orbit');
expect(gd.layout.dragmode).toBe(undefined);
expect(gd.layout.dragmode).toBe('zoom'); // fallback for multi-type subplots
expect(gd._fullLayout.dragmode).toBe('zoom');
expect(buttonTurntable.isActive()).toBe(false);
expect(buttonOrbit.isActive()).toBe(true);
Expand Down
32 changes: 31 additions & 1 deletion test/jasmine/tests/hover_label_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1375,6 +1375,8 @@ describe('hover on fill', function() {
var gd = createGraphDiv();

Plotly.plot(gd, mock.data, mock.layout).then(function() {
expect(gd._fullLayout.hovermode).toBe('closest');

// hover over a point when that's closest, even if you're over
// a fill, because by default we have hoveron='points+fills'
return assertLabelsCorrect([237, 150], [240.0, 144],
Expand Down Expand Up @@ -1402,7 +1404,35 @@ describe('hover on fill', function() {
}).then(function() {
// then make sure we can still select a *different* item afterward
return assertLabelsCorrect([237, 218], [266.75, 265], 'trace 1');
}).then(done);
})
.catch(fail)
.then(done);
});

it('should act like closest mode on ternary when cartesian is in compare mode', function(done) {
var mock = Lib.extendDeep({}, require('@mocks/ternary_fill.json'));
var gd = createGraphDiv();

mock.data.push({y: [7, 8, 9]});
mock.layout.xaxis = {domain: [0.8, 1], visible: false};
mock.layout.yaxis = {domain: [0.8, 1], visible: false};

Plotly.plot(gd, mock.data, mock.layout).then(function() {
expect(gd._fullLayout.hovermode).toBe('x');

// hover over a point when that's closest, even if you're over
// a fill, because by default we have hoveron='points+fills'
return assertLabelsCorrect([237, 150], [240.0, 144],
'trace 2Component A: 0.8Component B: 0.1Component C: 0.1');
}).then(function() {
// hovers over fills
return assertLabelsCorrect([237, 170], [247.7, 166], 'trace 2');
}).then(function() {
// hover on the cartesian trace in the corner
return assertLabelsCorrect([363, 122], [363, 122], 'trace 38');
})
.catch(fail)
.then(done);
});
});

Expand Down
Loading