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.

Sign up for GitHub

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

Jump to bottom

[Dev] Enable title in tiles #232

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

[Dev] Enable title in tiles #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80e620b
initial poc for enabling tiles titles
nadijagraca Dec 19, 2023
e98b4a3
adding css for tile title
nadijagraca Dec 20, 2023
a97939b
adding styling to tile title
nadijagraca Dec 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 37 additions & 20 deletions vizro-core/src/vizro/models/_components/graph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import logging
from typing import List, Literal

from dash import ctx, dcc
from dash import ctx, dcc, html
from dash.exceptions import MissingCallbackContextException
from plotly import graph_objects as go

Expand Down Expand Up @@ -34,6 +34,7 @@ class Graph(VizroBaseModel):
type: Literal["graph"] = "graph"
figure: CapturedCallable = Field(..., import_path=px)
actions: List[Action] = []
_title: str = PrivateAttr()

# Component properties for actions and interactions
_output_property: str = PrivateAttr("figure")
Expand Down Expand Up @@ -70,33 +71,49 @@ def __getitem__(self, arg_name: str):
return self.type
return self.figure[arg_name]

@_log_call
def pre_build(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it easy to do this in a validator for title rather than pre_build? If so then I think we should prefer that.

try:
self._title = self.figure._CapturedCallable__bound_arguments["title"]
self.figure._CapturedCallable__bound_arguments["title"] = None
Comment on lines +77 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self._title = self.figure._CapturedCallable__bound_arguments["title"]
self.figure._CapturedCallable__bound_arguments["title"] = None
self._title = self.figure["title"]

You should never need to access a property that has its name mangled like that. In this case we actually have a public method which does the same thanks to CapturedCallable.__getitem__.

Best not to modify the CapturedCallable in any way here if it's possible to avoid also. What I'd suggest instead is that you modify this bit of code in __call__:

        # Remove top margin if title is provided
        if fig.layout.title.text is None:
            fig.update_layout(margin_t=24)

to something like this:

        fig.layout.title.text = None
        fig.update_layout(margin_t=24)

Note one limitation of doing this is that users will no longer be able to edit the figure title through a parameter, but I'm ok with that.

I just thought through various different ways to handle this (e.g. should we take the title from CapturedCallable["title"] or from fig.layout.title.text?). Each has various pros and cons and none of them are perfect. The solution I propose here sounds best to me but please say if you think I forgot a better solution @maxschulz-COL since I remember discussing this with you before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very interesting. Just to jog my memory a bit, did we decide on implementing the tiles as presented here, ie as part of the Graph model?

I also remember that we discussed the title bit, but I cannot recall if we came to a conclusion.

As for the two cases:

  • take the title from CapturedCallable["title"]: this has the fundamental problem of making this functionality argument dependent. What if I create a custom chart and call my title argument scatter_title. In this case this would not work right?
  • take it from fig.layout.title.text: This seems to me the more robust approach, as no matter how I call my arguments, the title of the layout should always be here, so this is my preferred approach at first thought

Now for the question of not updating titles through parameters, I am undecided. On the one hand I think this is a functionality that may be hardly if ever used, on the other hand, I think this introduces a dangerous pattern of not allowing parameters to target any argument.

I think we previously discussed also a slightly less automated version:

  • allow the graph model a title parameter that is public
  • do not delete the chart title, but have the user remove it manually (set to None) if they do not want two titles
  • this has other downsides, but thought I'd mention it

Copy link
Contributor

Choose a reason for hiding this comment

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

Very interesting. Just to jog my memory a bit, did we decide on implementing the tiles as presented here, ie as part of the Graph model?

Yes, exactly 👍

As for the two cases:

  • take the title from CapturedCallable["title"]: this has the fundamental problem of making this functionality argument dependent. What if I create a custom chart and call my title argument scatter_title. In this case this would not work right?

Yes, that would not work. For me this is ok so long as all the plotly.express graph have the argument called title?

  • take it from fig.layout.title.text: This seems to me the more robust approach, as no matter how I call my arguments, the title of the layout should always be here, so this is my preferred approach at first thought

Agree this is a more robust approach because it also handles the case that you do not specify the title at all through an argument but still have a graph title, like this:

@capture("graph"):
def custom_chart():
    return px.scatter(..., title="blah")

Here if you tried to extract title by fig["title"] it wouldn't give you anything, but fig.layout.title.text would.

The big problem with approach is that in order to get fig.layout.title.text, you have to actually do the __call__ to generate the figure first. This is a relatively expensive operation (e.g. if you have a lot of data), and you're basically making an entire figure just to find the title. Then you throw away the whole graph and have to rebuild the whole thing anyway. tbh this is ok but just feels kind of wasteful when in 99% of cases I guess that the title of the graph has been specified through the argument directly.

So it's doing something which is quick and easy and works in I guess 99% of cases vs. something which is wasteful but works in 100% of cases. I guess we could have some hybrid approach where first we try to do fig["title"], and only if that doesn't exist do the actual __call__ and look in fig.layout.title.text. But probably that's just overcomplicating it it's just an edge case.

Now for the question of not updating titles through parameters, I am undecided. On the one hand I think this is a functionality that may be hardly if ever used, on the other hand, I think this introduces a dangerous pattern of not allowing parameters to target any argument.

I also don't much like it as a constraint, but I think the alternative will just be more complicated than is worth it. There are workarounds if people really want to do this (write an action/callback), but it wouldn't work as a pure Parameter.

This problem is equal for both approaches btw (using fig["title"] and fig.layout.title.text).

I think we previously discussed also a slightly less automated version:

  • allow the graph model a title parameter that is public
  • do not delete the chart title, but have the user remove it manually (set to None) if they do not want two titles
  • this has other downsides, but thought I'd mention it

This is also ok by me. It's certainly an easy solution and would work well (apart from the Parameter thing which is still not possible). Just it feels like in 99% of cases we can easily solve this for the user already without them having to move the title value from their plotting function to Graph.title. This is why I think I'd favour the "quick and easy look up fig["title"]" which makes people's lives easier and works most of the time, even though it's not perfect. We could also make Graph.title public (probably good idea anyway for consistency with Table) and take precedence over fig["title"] when it is set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good points! And very important point on the performance.

I don't understand one thing though

This is also ok by me. It's certainly an easy solution and would work well (apart from the Parameter thing which is still not possible).
Why is that? if we do not delete the chart title, but leave everything as is, this should behave like before?

All things considered I feel we may want to start with the third approach, and then we can still go either way if it is actually important. Maybe we end up liking it enough such that we do not need to force the automation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes sorry, you're right that the 3rd approach does indeed allow changing the title using a Parameter so long as you specify it through the plotting function and not Graph.title. That is a definite advantage of that approach.

The biggest disadvantage I guess is that it looks a bit weird/confusing to have a title field in two places (Graph and the plotting function), and that they render a bit differently on the screen.

I'm happy to go with this as the solution anyway and see if anyone finds it awkward or if it's actually fine in reality. It's definitely the easiest solution.

except KeyError:
self._title = None

@_log_call
def build(self):
# The empty figure here is just a placeholder designed to be replaced by the actual figure when the filters
# etc. are applied. It only appears on the screen for a brief instant, but we need to make sure it's
# transparent and has no axes so it doesn't draw anything on the screen which would flicker away when the
# graph callback is executed to make the dcc.Loading icon appear.
return dcc.Loading(
dcc.Graph(
id=self.id,
figure=go.Figure(
layout={
"paper_bgcolor": "rgba(0,0,0,0)",
"plot_bgcolor": "rgba(0,0,0,0)",
"xaxis": {"visible": False},
"yaxis": {"visible": False},
}

graph_div = html.Div(
children=[
html.P(self._title, className="tile-title"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess html.H3 or whatever level it's at might be better than html.P here?

dcc.Loading(
dcc.Graph(
id=self.id,
figure=go.Figure(
layout={
"paper_bgcolor": "rgba(0,0,0,0)",
"plot_bgcolor": "rgba(0,0,0,0)",
"xaxis": {"visible": False},
"yaxis": {"visible": False},
}
),
config={
"autosizable": True,
"frameMargins": 0,
"responsive": True,
},
className="chart_container",
),
color="grey",
parent_className="loading-container",
),
config={
"autosizable": True,
"frameMargins": 0,
"responsive": True,
},
className="chart_container",
),
color="grey",
parent_className="loading-container",
],
style={"height": "96%"}, # inline style to be removed
)
return graph_div

@staticmethod
def _update_theme(fig: go.Figure, theme_selector: bool):
Expand Down
9 changes: 9 additions & 0 deletions vizro-core/src/vizro/static/css/layout.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,12 @@ div.dashboard_container .custom-tooltip {
text-wrap: wrap;
white-space: pre-wrap;
}

.tile-title {
font-size: 20px;
color: var(--text-primary);
opacity: 0.85;
font-weight: normal;
white-space: pre;
padding-left: 80px;
}