Skip to content

Re-add support for passing title as string #7230

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

Closed
wants to merge 6 commits into from
Closed
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
6 changes: 6 additions & 0 deletions src/components/colorbar/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,4 +228,10 @@ module.exports = overrideAll({
].join(' ')
}
},
_deprecated: {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about dropping the deprecation here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. I see then it would collide with the the title which is declared as an object.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the best way of handing these cases today is to change the title valType to be any and then support both String and Object types so that when title is a string title.text is set to the string.
How about opening a new PR with this approach?

Copy link
Contributor Author

@emilykl emilykl Oct 17, 2024

Choose a reason for hiding this comment

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

@archmoj That sounds fine but I'm unsure about the attributes syntax for accepting both an object (with specific nested keys) and a string... this is the attribute definition right now for title in colorbar:

    title: {
        text: {
            valType: 'string',
            description: 'Sets the title of the color bar.'
        },
        font: fontAttrs({
            description: 'Sets this color bar\'s title font.'
        }),
        side: {
            valType: 'enumerated',
            values: ['right', 'top', 'bottom'],
            description: [
                'Determines the location of color bar\'s title',
                'with respect to the color bar.',
                'Defaults to *top* when `orientation` if *v* and ',
                'defaults to *right* when `orientation` if *h*.',
            ].join(' ')
        }
    },

My point is that there doesn't seem to be a place to define valType for title if we are also defining nested attributes under title. Do you have a suggestion?

Copy link
Contributor

@archmoj archmoj Oct 17, 2024

Choose a reason for hiding this comment

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

Let's see what @alexcjohnson would suggest here.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about keeping the valType as Object with and add an extra option for this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or perhaps we should just handle this String option on the plotly.py side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@archmoj Do you have an example of an extra option being used elsewhere in the codebase?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this: https://github.com/plotly/plotly.js/pull/6990/files#r1591049384
Instead of extra you may add another option to coerce and name it stringOk or something else.

title: {
valType: 'string',
description: 'It is recommended to use the color bar\'s `title.text` attribute instead.'
}
}
}, 'colorbars', 'from-root');
53 changes: 53 additions & 0 deletions src/plot_api/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,30 @@ exports.cleanLayout = function(layout) {

// prune empty domain arrays made before the new nestedProperty
if(emptyContainer(ax, 'domain')) delete ax.domain;

cleanTitle(ax);
} else if(polarAttrRegex && polarAttrRegex.test(key)) {
// modifications for polar

var polar = layout[key];
cleanTitle(polar.radialaxis);
} else if(ternaryAttrRegex && ternaryAttrRegex.test(key)) {
// modifications for ternary

var ternary = layout[key];
cleanTitle(ternary.aaxis);
cleanTitle(ternary.baxis);
cleanTitle(ternary.caxis);
} else if(sceneAttrRegex && sceneAttrRegex.test(key)) {
// modifications for 3D scenes

var scene = layout[key];

// clean axis titles
cleanTitle(scene.xaxis);
cleanTitle(scene.yaxis);
cleanTitle(scene.zaxis);

}
}

Expand Down Expand Up @@ -134,6 +158,9 @@ exports.cleanLayout = function(layout) {
}
}

// clean plot title
cleanTitle(layout);

/*
* Moved from rotate -> orbit for dragmode
*/
Expand All @@ -159,6 +186,25 @@ function cleanAxRef(container, attr) {
}
}

/**
* Cleans up old title-as-string attribute by moving a string passed as `title` to `title.text`.
*
* @param {Object} titleContainer - an object potentially including a `title` attribute
* which may be a string
*/
function cleanTitle(titleContainer) {
if(titleContainer) {
// title -> title.text
// (although title used to be a string attribute,
// numbers are accepted as well)
if(typeof titleContainer.title === 'string' || typeof titleContainer.title === 'number') {
titleContainer.title = {
text: titleContainer.title
};
}
}
}

/*
* cleanData: Make a few changes to the data for backward compatibility
* before it gets used for anything. Modifies the data traces users provide.
Expand Down Expand Up @@ -293,6 +339,13 @@ exports.cleanData = function(data) {
delete trace.autobiny;
delete trace.ybins;
}

cleanTitle(trace);
if(trace.colorbar) cleanTitle(trace.colorbar);
if(trace.marker && trace.marker.colorbar) cleanTitle(trace.marker.colorbar);
if(trace.line && trace.line.colorbar) cleanTitle(trace.line.colorbar);
if(trace.aaxis) cleanTitle(trace.aaxis);
if(trace.baxis) cleanTitle(trace.baxis);
}
};

Expand Down
38 changes: 38 additions & 0 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -1392,6 +1392,8 @@ function _restyle(gd, aobj, traces) {
var eventData = Lib.extendDeepAll({}, aobj);
var i;

cleanTitleAttribute(aobj);

// initialize flags
var flags = editTypes.traceFlags();

Expand Down Expand Up @@ -1693,6 +1695,40 @@ function _restyle(gd, aobj, traces) {
};
}

/**
* When a string is passed to the title attribute, convert it
* to title.text instead.
*
* This is needed for the update mechanism to determine which
* subroutines to run based on the actual attribute
* definitions (that don't include the deprecated ones).
*
* E.g. Maps {'xaxis.title': 'A chart'} to {'xaxis.title.text': 'A chart'}.
*
* @param aobj
*/
function cleanTitleAttribute(aobj) {
var oldAxisTitleRegex = Lib.counterRegex('axis', '\.title', false, false);
var colorbarRegex = /colorbar\.title$/;
var keys = Object.keys(aobj);
var i, key, value;

for(i = 0; i < keys.length; i++) {
key = keys[i];
value = aobj[key];

if((key === 'title' || oldAxisTitleRegex.test(key) || colorbarRegex.test(key)) &&
(typeof value === 'string' || typeof value === 'number')) {
replace(key, key.replace('title', 'title.text'));
}
}

function replace(oldAttrStr, newAttrStr) {
aobj[newAttrStr] = aobj[oldAttrStr];
delete aobj[oldAttrStr];
}
}

/**
* relayout: update layout attributes of an existing plot
*
Expand Down Expand Up @@ -1878,6 +1914,8 @@ function _relayout(gd, aobj) {

keys = Object.keys(aobj);

cleanTitleAttribute(aobj);

// look for 'allaxes', split out into all axes
// in case of 3D the axis are nested within a scene which is held in _id
for(i = 0; i < keys.length; i++) {
Expand Down
8 changes: 8 additions & 0 deletions src/plots/cartesian/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -1230,4 +1230,12 @@ module.exports = {
].join(' ')
},
editType: 'calc',

_deprecated: {
title: {
valType: 'string',
editType: 'ticks',
description: 'It is recommended to use the layout\'s `title.text` attribute instead.'
},
}
};
3 changes: 3 additions & 0 deletions src/plots/gl3d/layout/axis_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,4 +121,7 @@ module.exports = overrideAll({
zeroline: axesAttrs.zeroline,
zerolinecolor: axesAttrs.zerolinecolor,
zerolinewidth: axesAttrs.zerolinewidth,
_deprecated: {
title: axesAttrs._deprecated.title
}
}, 'plot', 'from-root');
7 changes: 7 additions & 0 deletions src/plots/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -451,4 +451,11 @@ module.exports = {
].join(' '),
editType: 'none'
}),
_deprecated: {
title: {
valType: 'string',
editType: 'layoutstyle',
description: 'It is recommended to use the layout\'s `title.text` attribute instead.'
}
}
};
4 changes: 4 additions & 0 deletions src/plots/polar/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ var radialAxisAttrs = {
},

editType: 'calc',

_deprecated: {
title: axesAttrs._deprecated.title
}
};

extendFlat(
Expand Down
3 changes: 3 additions & 0 deletions src/plots/ternary/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ var ternaryAxesAttrs = {
'all the minima set to zero.'
].join(' ')
},
_deprecated: {
title: axesAttrs._deprecated.title
}
};

var attrs = module.exports = overrideAll({
Expand Down
10 changes: 9 additions & 1 deletion src/traces/carpet/axis_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -451,5 +451,13 @@ module.exports = {
editType: 'calc',
description: 'The stride between grid lines along the axis'
},
editType: 'calc'
editType: 'calc',

_deprecated: {
title: {
valType: 'string',
editType: 'calc',
description: 'It is recommended to use the axis\'s `title.text` attribute instead.'
}
},
};
9 changes: 9 additions & 0 deletions src/traces/pie/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,4 +292,13 @@ module.exports = {
'or an array to highlight one or more slices.'
].join(' ')
},

_deprecated: {
title: {
valType: 'string',
dflt: '',
editType: 'calc',
description: 'It is recommended to use the pie\'s `title.text` attribute instead.'
}
}
};
18 changes: 18 additions & 0 deletions test/jasmine/tests/funnelarea_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,24 @@ describe('Funnelarea traces', function() {
.then(done, done.fail);
});

it('should be able to restyle title despite using deprecated title-as-string', function(done) {
Plotly.newPlot(gd, [{
type: 'funnelarea',
values: [1, 2, 3],
title: 'yo',
}])
.then(function() {
_assertTitle('base', 'yo', 'rgb(68, 68, 68)');
return Plotly.restyle(gd, {
title: 'oy',
});
})
.then(function() {
_assertTitle('base', 'oy', 'rgb(68, 68, 68)');
})
.then(done, done.fail);
});

it('should be able to react with new text colors', function(done) {
Plotly.newPlot(gd, [{
type: 'funnelarea',
Expand Down
18 changes: 18 additions & 0 deletions test/jasmine/tests/pie_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,24 @@ describe('Pie traces', function() {
.then(done, done.fail);
});

it('should be able to restyle title despite using deprecated title-as-string', function(done) {
Plotly.newPlot(gd, [{
type: 'funnelarea',
values: [1, 2, 3],
title: 'yo',
}])
.then(function() {
_assertTitle('base', 'yo', 'rgb(68, 68, 68)');
return Plotly.restyle(gd, {
title: 'oy',
});
})
.then(function() {
_assertTitle('base', 'oy', 'rgb(68, 68, 68)');
})
.then(done, done.fail);
});

it('should be able to react with new text colors', function(done) {
Plotly.newPlot(gd, [{
type: 'pie',
Expand Down
24 changes: 24 additions & 0 deletions test/jasmine/tests/plot_api_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,30 @@ describe('Test plot api', function() {
.then(assertSizeAndThen(543, 432, true, 'final back to autosize'))
.then(done, done.fail);
});

it('passes update data back to plotly_relayout unmodified ' +
'even if deprecated title-as-string has been used', function(done) {
Plotly.newPlot(gd, [{y: [1, 3, 2]}])
.then(function() {
gd.on('plotly_relayout', function(eventData) {
expect(eventData).toEqual({
title: 'Plotly chart',
'xaxis.title': 'X',
'yaxis.title': 'Y',
'polar.radialaxis.title': 'Radial'
});
done();
});

return Plotly.relayout(gd, {
title: 'Plotly chart',
'xaxis.title': 'X',
'yaxis.title': 'Y',
'polar.radialaxis.title': 'Radial'
});
})
.then(done, done.fail);
});
});

describe('Plotly.relayout subroutines switchboard', function() {
Expand Down
Loading