Skip to content
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

882 - Chart Title Alignment #3276

Merged
merged 50 commits into from Nov 29, 2018
Merged

882 - Chart Title Alignment #3276

merged 50 commits into from Nov 29, 2018

Conversation

@rmoestl
Copy link
Contributor

@rmoestl rmoestl commented Nov 21, 2018

This PR adds the capability to align the main plot title as requested in issue #882.

New title attributes structure
Besides the introduction of the discussed alignment options, this PR also transitions to a new title attribute structure. layout.title has become layout.title.text and layout.titlefont has become layout.title.font. The same goes for any other title definitions within layout, e.g. xaxis, yaxis, polar.radialaxis and so on.

The structure of title attributes within data remains unchanged as it won't have added any significant value to users compared to the additional effort this would have taken (e.g. transitioning pie).

To be discussed

  • Two new TODOs were added that probably need a closer look by a core maintainer.
  • The new title structure has changed event data of plotly_relayout which especially showed in two broken and thus updated tests in legend_test.js. Users are still able to use the old attributes structure as an input to Plotly.relayout and Plotly.update. But any handlers listening on plotly_relayout events, that assume the old title structure, need to be updated.
  • Lib.warn calls in cleanLayout have not been added so far since the original issue is about two years old as of writing and every other clean-up code in cleanLayout does not yet emit warnings.
rmoestl added 30 commits Nov 21, 2018
- Not cleaned up.
- Padding attributes missing.
- `auto` values missing.
- Current position means the title's baseline sits in the middle of the
  to margin.
- For initial render and update through relayout/update.
- colorbar is using the titles component to draw its title and
  therefore needed to adapt to the attribute structure assumed by the
  titles component.
- Adapt layout attributes.
- Extend compatibility code in `cleanLayout`.
- Adapt polar drawing code.
- Adapt tests being based on old title attr structure.
- Adapt layout attributes.
- Extend compatibility code in `cleanLayout`.
- Adapt ternary drawing code.
- Adapt tests being based on old title attr structure.
- Test relayouting title attributes.
- Adapt layout attributes.
- Extend compatibility code in `cleanLayout`.
- Further being splom, validate and template tests.
- Special coerce logic for y because it can be 'auto' also.
- Eliminate an unnecessary else block.
- Bug: Using an attribute string representing the deprecated title
  structure to unset a value wasn't supported.
- Bug: if only a `titlefont` but no `title` was set, `cleanTitle`
  would not create the new title structure.
function coerceTitleY() {
if(layoutIn.title && isNumeric(layoutIn.title.y)) {
var propOut = Lib.nestedProperty(layoutOut, 'title.y');
var opts = Lib.nestedProperty(plots.layoutAttributes, 'title.y').get();
Copy link
Contributor

@alexcjohnson alexcjohnson Nov 26, 2018

If we keep this, it doesn't need to be nestedProperty - plots.layoutAttributes.title.y would work, right?

Copy link
Contributor Author

@rmoestl rmoestl Nov 26, 2018

You're right. Lib.valObjectMeta.number.coerceFunction used to check the defined min/max bounds but the removal of min/max in the attribute definition rendered that obsolete (see above) 🤦‍♂️

Copy link
Contributor Author

@rmoestl rmoestl Nov 28, 2018

See commit 5fb7b8f

@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Nov 26, 2018

Should we also change title / titlefont and other title* trace attribute to title.text / title.font / ... in this PR?

carpet.(a|b)axis.title seems to me like it maps naturally to cartesian axis titles, so I feel like it would be confusing NOT to move this to the new structure. And at that point seems to me we should convert them all - ie any remaining title -> title.text and title* -> title.*

rmoestl added 6 commits Nov 26, 2018
- Swichting the valType to 'number' spares the custom coerce function.
- In addition fixes the missing min and max bounds for y.
- This transitions all traces depending on `colorbar` as well.
- Also remove unnecessary code in carpet implementation.
@rmoestl
Copy link
Contributor Author

@rmoestl rmoestl commented Nov 28, 2018

And at that point seems to me we should convert them all - ie any remaining title -> title.text and title* -> title.*

Converted pie, carpet and colorbar. colorbar component is used in a lot of trace types of course.

- Reason: having no default at all for `title.text` would lead to a
  non-existing `title` container (through a call to
  `relinkPrivateKeys` deep down the stack) eventually and as consequence
  would require to safeguard access to `[ab]axis.title.text` by checking
  `[abaxis.title]` is defined before.
if(!containerOut.title.text) {
delete containerOut.title.font;
delete containerOut.title.offset;
}
Copy link
Contributor

@alexcjohnson alexcjohnson Nov 29, 2018

Is there some reason it doesn't work to invert this pattern? ie only coerce these attributes if we have a title, rather than always coercing them then deleting if there's no title?

There is a lot of this pattern above - coerce everything and then delete - and it's needed when a value provided to one of these other attributes affects the default of the controlling attribute - like if there's an explicit start line color or width we default to show the start line, so we need to coerce startlinecolor and startlinewidth before we can correctly coerce startline but then if start line was explicitly hidden we need to delete color and width. But I don't think that applies here.

Copy link
Contributor Author

@rmoestl rmoestl Nov 29, 2018

Is there some reason it doesn't work to invert this pattern?

I don't think so, but I can't say for sure TBH. What could be a reason for checking for an empty title and deleting title attributes at the end? In theory, every call to coerce could clear containerOut.title. But since there's no comment on the "why", let's reverse the logic as you suggested and keep 🤞 we haven't broke anything. Does that sound like a plan?

coerce('titleside');
coerce('title.text', layout._dfltTitle.colorbar);
Lib.coerceFont(coerce, 'title.font', layout.font);
coerce('title.side');
Copy link
Contributor

@alexcjohnson alexcjohnson Nov 29, 2018

Oh hmm, I was going to say we should not coerce title.font and title.side here but because of _dfltTitle you'd have to explicitly set title.text = '' to make these irrelevant, and even then in principle in editable: true mode we would need these attributes. So I guess here - and everywhere else the titles are editable within the plot - it's correct as is. And the only places that doesn't apply are carpet (discussed elsewhere) and pie (already handled correctly) 🎉

Copy link
Contributor Author

@rmoestl rmoestl Nov 29, 2018

I agree. That was my concern as well (plus my guess that we might need font to calculate things like automargin even if we don't have a title set). But I resisted to raise that because I haven't done the research in the code to underpin my claim.

@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Nov 29, 2018

Beautiful. As I was giving the code a final look I saw a few extra "oh wait, what about..." only to find you'd already addressed and thoroughly tested them all. Love it.

💃

@etpinard
Copy link
Contributor

@etpinard etpinard commented Nov 29, 2018

I'm going to add a second 💃 here. Amazing work @rmoestl !

@rmoestl rmoestl merged commit db66ff1 into master Nov 29, 2018
8 checks passed
@rmoestl rmoestl deleted the 882-chart-title-alignment branch Nov 29, 2018
@rmoestl
Copy link
Contributor Author

@rmoestl rmoestl commented Nov 29, 2018

🎉
Thanks to both of you, @etpinard and @alexcjohnson! As always, I really appreciated your thorough review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants