[Tidy] Move figure layout dashboard overrides into a template

[antonymilne]

Description

Completely closes
https://github.com/McK-Internal/vizro-internal/issues/1208. This should now be the last thing we need to do on graph templates 🥳

Now that templates are moving entirely to the clientside there's a much cleaner way to apply the figure layout updates:

• better to apply them through a template rather than fig.layout so there's no need to do the if fig.layout.property is None check.
• vizro_light and vizro_dark are unchanged and still optimised for use outside dashboards. Now we merge in a new theme dashboard_template_overrides when they're applied in the dashboard. User changes to vizro_light/dark are still maintained since we just merge the extra dashboard_template_overrides on top of them. The new dashboard_template_overrides is not public and so not registered in pio.templates
• the vizro_light/dark + dashboard_template_overrides is optimised for charts with no titles, so that all the weird custom logic that alters fig.layout now only applies when a title is set. This will probably be removed soon.

This is just refactoring. Nothing should have changed here either inside or outside dashboard.

@huong-li-nguyen please could you double check I didn't change anything accidentally here? As I understand it, this should be the behaviour both before and after this PR:

Standalone chart

title_pad_l = 24
title_pad_r = 24
title_pad_t = 24
title_pad_b = 0

margin_l = 80
margin_r = 24
margin_t = 64
margin_b = 64

Chart in dashboard with no title

Same as standalone chart except title_pad_l, title_pad_r, margin_l, margin_t different:

title_pad_l = 0
title_pad_r = 0
title_pad_t = 24
title_pad_b = 0

margin_l = 24
margin_r = 24
margin_t = 24
margin_b = 64

Chart in dashboard with title and no subtitle

Same as chart in dashboard with no title except title_pad_t = 7.

Chart in dashboard with title and no subtitle

Same as chart in dashboard with no title except margin_t = 64 (what it was in standalone chart)

Screenshot

Notice

• I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
  • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
  • I have not referenced individuals, products or companies in any commits, directly or indirectly.
  • I have not added data or restricted code in any commits, directly or indirectly.

[huong-li-nguyen]

This looks great! 🚀 Very satisfying to see some of the code go 😄

It's all good from my side. I didn't manually test anything, but if the unit tests and screenshots tests have passed + the scratch example looks fine, it should be all good!

[petar-qb]

Thanks for the code and the tests refactoring. ⚡ It will be much easier to maintain themes with these changes.

Here's a screenshot of the scratch dev app. Are we satisfied with the current margins and padding? The reason I ask is that the titles of the graphs, those with subtitles, seem to be positioned too far from the top border of their container. Do you think we should adjust this spacing?

[antonymilne]

Great observation @petar-qb, thanks for the careful testing 👍

My guess this is because the title_pad_t=7 that we apply when there's a subtitle is just a rough estimate and it's impossible to get it better than this. But @huong-li-nguyen will know better:

1. has the alignment got worse here due to my changes or did it always look like this?
2. should we try and tweak title_pad_t to align better?

Unless @huong-li-nguyen thinks otherwise then I'd say we just leave it how it is because we're going to be removing the whole optimise_fig_layout_for_dashboard soon in favour of new fields Graph.title, Graph.header, Graph.footer (and similarly for other models) which will be the new, improved way of doing titles and subtitles. When that change is made we won't have any alignment issues any more and we won't need to hack around with title_pad_t at all.

[huong-li-nguyen]

Great observation @petar-qb, thanks for the careful testing 👍

My guess this is because the title_pad_t=7 that we apply when there's a subtitle is just a rough estimate and it's impossible to get it better than this. But @huong-li-nguyen will know better:

1. has the alignment got worse here due to my changes or did it always look like this?
2. should we try and tweak title_pad_t to align better?

Unless @huong-li-nguyen thinks otherwise then I'd say we just leave it how it is because we're going to be removing the whole optimise_fig_layout_for_dashboard soon in favour of new fields Graph.title, Graph.header, Graph.footer (and similarly for other models) which will be the new, improved way of doing titles and subtitles. When that change is made we won't have any alignment issues any more and we won't need to hack around with title_pad_t at all.

Just leave it as is. It's really icky to get this right, because the graph.title is coming from Plotly so the normal margin / padding specifications don't apply as if this were a completely new html container 😅 So 7px is just a guess. It's not pitch-perfect. I don't think many people use subtitles anyway and as @antonymilne says, I'll change that very soon anyway.