-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
| ].join(' ') | ||
| } | ||
| }, | ||
| _deprecated: { |
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.
How about dropping the deprecation here?
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.
Hmm.. I see then it would collide with the the title which is declared as an object.
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.
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?
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.
@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?
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.
Let's see what @alexcjohnson would suggest here.
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.
How about keeping the valType as Object with and add an extra option for this case?
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.
Or perhaps we should just handle this String option on the plotly.py side?
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.
@archmoj Do you have an example of an extra option being used elsewhere in the codebase?
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.
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.
|
We should possibly support this option on places e.g. |
Agreed, it makes sense to be consistent, I'll look into how much work this would be. |
|
Revised in #7262. |
| }, | ||
| { | ||
| desc: 'despite passing title only as a string (backwards-compatibility)', | ||
| update: { | ||
| title: NEW_TITLE, | ||
| xaxis: {title: NEW_XTITLE}, | ||
| yaxis: {title: NEW_YTITLE} | ||
| } | ||
| }, | ||
| { | ||
| desc: 'despite passing title only as a string using string attributes ' + | ||
| '(backwards-compatibility)', | ||
| update: { | ||
| title: NEW_TITLE, | ||
| 'xaxis.title': NEW_XTITLE, | ||
| 'yaxis.title': NEW_YTITLE | ||
| } |
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.
titleproperty (removed in Drop support for passing a string to thetitleattribute, and drop support for deprecated attributestitlefont,titleposition,titleside, andtitleoffset#7212)titlefont,titleoffset) remain removedtitlestring totitle.textinternally