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

[Feat] Add info icon to dashboard title #1053

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

nadijagraca
Copy link
Contributor

@nadijagraca nadijagraca commented Mar 5, 2025

Description

POC implementation

API needs to be agreed

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
Copy link
Contributor

huong-li-nguyen commented Mar 5, 2025

I just looked at the design for now as you asked :)

I think overall it looks good, I would just ensure:

  • It's center-aligned with the title
  • Add color: var(--text-primaryHover) to the icon on hover
  • Change font-hierarchy inside tooltip so the text is not bigger than the actual title (it probably should use h3/h4 max or just use p)
  • Optimally, the font-size needs to be updated based on the title it is attached to (feel free to discuss this one with @Joseph-Perkins ) as well. So I feel the current font size is too small for the dashboard title.
  • Doesn't have to be added in this version, but I feel a "filled" and "outlined" version would be nice. Currently you have outlined implemented, but a filled one could also look nice 👍

See this article: https://hanjing.medium.com/best-practice-of-i-icon-ab6713e3aac9

  • For headers: 20x20 pixels - 24x24
  • For body text: 16x16 pixels - 18x18 pixels

Overall question: I am not sure if I like the position of this icon, but this is personal preference 😬 I think in general it makes sense to have them close to the title, so for any other title I would agree it should be right next to it. However, for this specific case, you have (icon on dashboard-level) I'd prefer it next to the toggle-switch.

Comment on lines 23 to 25
title: str = Field(default="", description="Dashboard title to appear on every page on top left-side.")
icon: bool = False
tooltip_text: str = Field(description="Markdown string to create card text that appears on icon hover.")
Copy link
Contributor

@antonymilne antonymilne Mar 5, 2025

Choose a reason for hiding this comment

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

Suggested change
title: str = Field(default="", description="Dashboard title to appear on every page on top left-side.")
icon: bool = False
tooltip_text: str = Field(description="Markdown string to create card text that appears on icon hover.")
text: str = Field(description="Dashboard title to appear on every page on top left-side.") # mandatory
icon: str = "info-mark" # optional, can take any Material icon and defaults to info
tooltip: str = Field(description="Markdown string to create card text that appears on icon hover.") # mandatory

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI @nadijagraca: @huong-li-nguyen, @maxschulz-COL, @Joseph-Perkins, @lingyielia and I just had a chat about this and think this would be the best API. Let me know if it makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I agree, avoiding the double title is good 👍

@nadijagraca nadijagraca marked this pull request as ready for review March 10, 2025 16:34
Copy link
Contributor

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2025-03-10 16:37:08 UTC
Commit: 93ee441

Compare the examples using the commit's wheel file vs the latest released version:

vizro-core/examples/scratch_dev

View with commit's wheel vs View with latest release

vizro-core/examples/dev/

View with commit's wheel vs View with latest release

vizro-core/examples/visual-vocabulary/

View with commit's wheel vs View with latest release

vizro-core/examples/tutorial/

View with commit's wheel vs View with latest release

vizro-ai/examples/dashboard_ui/

View with commit's wheel vs View with latest release

@nadijagraca
Copy link
Contributor Author

Currently, vm.Title is designed to return the dashboard title component, which includes both the title and an icon (with a tooltip).
As agreed, at the dashboard level, the icon should be positioned on the right, next to the theme toggle. However, if we want to reuse vm.Title in other components that also have a title property, some adjustments are needed to make the icon positioning more flexible.
Some of the possible approaches:

  1. Via private property: Introduce a private property, _icon_position , which determines the icon's position based on where vm.Title is being used.

  2. Expose arguments: This could possibly be implemented in two ways:

    • Add properties like icon_size and icon_position directly to the vm.Title model.
    • Create a separate vm.Icon model, which would have size and position arguments.
  3. Have one standard size and position of the icon, and advise users to use custom css to alter size/position/any other css property of the icon.

@antonymilne @Joseph-Perkins I would like to hear your thoughts. These changes can be implemented in this PR, or in one of the following PRs, or not implemented at all.

@petar-qb petar-qb self-requested a review March 11, 2025 07:19
@petar-qb petar-qb changed the title [Dev] Add info icon to dashboard title [Feat] Add info icon to dashboard title Mar 11, 2025
Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

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

Great work @nadijagraca!! If we don't change the configuration API, this will be merged very soon 🚀

Comment on lines +38 to +50
return html.Div(
id="dashboard-title",
children=[
html.H2(id="dashboard-title-text", children=self.text),
html.Span(self.icon, className="material-symbols-outlined", id=f"{self.id}-icon"),
dbc.Tooltip(
id=f"{self.id}-tooltip",
children=dcc.Markdown(self.tooltip, dangerously_allow_html=True, id="dashboard-title-markdown"),
placement="left",
target=f"{self.id}-icon",
),
],
)
Copy link
Contributor

Choose a reason for hiding this comment

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

When we enable using vm.Title in other components (like vm.Page, vm.Container, vm.Checklist...), we won't be able to hardcode any of those IDs returned from the vm.Title.build method (due to duplication ID error). Starting from that point of thinking I guess that we should adjust this code section to something like:

Suggested change
return html.Div(
id="dashboard-title",
children=[
html.H2(id="dashboard-title-text", children=self.text),
html.Span(self.icon, className="material-symbols-outlined", id=f"{self.id}-icon"),
dbc.Tooltip(
id=f"{self.id}-tooltip",
children=dcc.Markdown(self.tooltip, dangerously_allow_html=True, id="dashboard-title-markdown"),
placement="left",
target=f"{self.id}-icon",
),
],
)
return html.Div(
id=self.id,
children=[
html.H2(id=f"{self.id}-title", children=self.text),
html.Span(self.icon, className="material-symbols-outlined", id=f"{self.id}-title-icon"),
dbc.Tooltip(
id=f"{self.id}-title-tooltip",
children=dcc.Markdown(self.tooltip, dangerously_allow_html=True, id="dashboard-title-markdown"),
placement="left",
target=f"{self.id}-title-icon",
),
],
)

There are many ID format variations we can use here instead and I'm okay with all of them as soon as we ensure that the vm.Title (from the duplication ID point of view) can already be used within other components. Also, let's have variants and extra arguments on our minds while redesigning this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The id is one of the issues we need to address if we want to reuse this within other components.

I agree that "dashboard-title" cannot be hardcoded in that case. However, when vm.Title is used specifically as the dashboard title, the id of the returned html.Div must be "dashboard-title", as it is referenced by ID in _dashboard.py.

def _arrange_page_divs(self, page_divs: _PageDivsType):
    logo_title = [page_divs["logo"], page_divs["logo-dark"], page_divs["logo-light"], page_divs["dashboard-title"]]

One possible solution is to explicitly pass vm.Title(id='dashboard-title' ...) when used as a dashboard title, and while keeping self.id when used within other components.

@@ -110,13 +112,17 @@ def pre_build(self):
self.pages[0].path = "/"
meta_img = self._infer_image("app") or self._infer_image("logo") or self._infer_image("logo_dark")

from vizro.models import Title

dashboard_title = self.title.text if isinstance(self.title, Title) else self.title
Copy link
Contributor

Choose a reason for hiding this comment

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

We have the if isinstance(dashboard.title, Title) check three times in the code (twice in _dashboard.py and once in _vizro.py). Is this a good sign that we should implicitly convert title: str -> title: vm.Title using the Pydantic validator? I know it's not the easiest change, but it's at least worth considering.

@Joseph-Perkins
Copy link
Contributor

Currently, vm.Title is designed to return the dashboard title component, which includes both the title and an icon (with a tooltip). As agreed, at the dashboard level, the icon should be positioned on the right, next to the theme toggle. However, if we want to reuse vm.Title in other components that also have a title property, some adjustments are needed to make the icon positioning more flexible. Some of the possible approaches:

  1. Via private property: Introduce a private property, _icon_position , which determines the icon's position based on where vm.Title is being used.

  2. Expose arguments: This could possibly be implemented in two ways:

    • Add properties like icon_size and icon_position directly to the vm.Title model.
    • Create a separate vm.Icon model, which would have size and position arguments.
  3. Have one standard size and position of the icon, and advise users to use custom css to alter size/position/any other css property of the icon.

@antonymilne @Joseph-Perkins I would like to hear your thoughts. These changes can be implemented in this PR, or in one of the following PRs, or not implemented at all.

I think it would make sense to go with the option where the design choices are applied implicitly, in order to remain opinionated about the design

so the private argument would seem to be preferable, instead of the user explicitly specifying size/position via the config or CSS?

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

Successfully merging this pull request may close these issues.

6 participants