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

Improve dashboard title #228

Merged
merged 11 commits into from
Dec 19, 2023
Merged

Improve dashboard title #228

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4def8ed
Improve dashboard title
antonymilne Dec 19, 2023
cfc3cf7
Lint and changelog
antonymilne Dec 19, 2023
e11f3b6
Update vizro-core/examples/default/app.py
antonymilne Dec 19, 2023
20dcd1a
Update example
antonymilne Dec 19, 2023
5cef34a
Revert example change
antonymilne Dec 19, 2023
c00fe4e
Update app title if dashboard.title set
antonymilne Dec 19, 2023
7f981fb
Update docs
antonymilne Dec 19, 2023
b5ea319
Update JSON schema comment
antonymilne Dec 19, 2023
708bb8e
Merge branch 'main' into feature/improve-dashboard-title
antonymilne Dec 19, 2023
638f0b0
Update vizro-core/examples/default/app.py
antonymilne Dec 19, 2023
403097a
Merge branch 'main' into feature/improve-dashboard-title
antonymilne Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions vizro-core/changelog.d/20231218_204302_antony.milne_improve_dashboard_title.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<!--
A new scriv changelog fragment.

Uncomment the section that is right (remove the HTML comment wrapper).
-->

<!--
### Highlights ✨

- A bullet item for the Highlights ✨ category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1))

-->
<!--
### Removed

- A bullet item for the Removed category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1))

-->

### Added

- When set, the dashboard title appears alongside the individual page title as the text labelling a browser tab. ([#228](https://github.com/mckinsey/vizro/pull/228))

huong-li-nguyen marked this conversation as resolved.
Show resolved Hide resolved
<!--
### Changed

- A bullet item for the Changed category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX ([#228](https://github.com/mckinsey/vizro/pull/228))

-->
<!--
### Deprecated

- A bullet item for the Deprecated category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1))

-->
<!--
### Fixed

- A bullet item for the Fixed category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1))

-->
<!--
### Security

- A bullet item for the Security category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1))

-->
1 change: 1 addition & 0 deletions vizro-core/examples/default/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ def create_home_page():


dashboard = vm.Dashboard(
title="My dashboard",
antonymilne marked this conversation as resolved.
Show resolved Hide resolved
pages=[
create_home_page(),
create_variable_analysis(),
Expand Down
8 changes: 6 additions & 2 deletions vizro-core/src/vizro/models/_dashboard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,13 @@ def pre_build(self):
# For now the homepage (path /) corresponds to self.pages[0].
# Note redirect_from=["/"] doesn't work and so the / route must be defined separately.
for order, page in enumerate(self.pages):
path = page.path if order else "/"
dash.register_page(
module=page.id, name=page.title, path=path, order=order, layout=partial(self._make_page_layout, page)
module=page.id,
name=page.title,
title=f"{self.title}: {page.title}" if self.title else page.title,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one line is the only functional change in the PR.

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 if people do want to overwrite it, they would overwrite the pre-build of the model here?

I was actually wondering how this works compared to the dash.app.title. If you set the dash.app.title, I noticed that it appears for a second and then it gets overwritten by anything defined in dash.register_page.

This might be worth mentioning in the docs if people try to overwrite it via the dash.app.title. This wouldn't actually work, because in the end it gets overwritten by the code here again. But I think this is also how it works in a pure Dash app?

app = Vizro(assets_folder="../assets").build(dashboard)
app.title = "My title"
app.run()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is indeed how it works in a pure Dash app also:

  1. Dash(title="X") corresponds to the property dash.app.title and sets the initial value of <title>X</title>
  2. but this only appears very briefly when you're using Dash pages, because it gets overwritten straight away by dash.page_registry[page.id]["title"]
  3. after this, the original app title=X has no effect on anything

So for Vizro users where Dash pages is always used, the title argument basically has no effect. @AnnMarieW please do correct me if I've missed something here.

We have possibly added to the confusion here because we have our own Dashboard.title property, and it's not obvious to a user what effect that has on anything. I considered before whether we should do dash.app.title = dashboard.title in Vizro.build() but decided it was basically pointless because, like you say, it just gets overwritten straight away. I think it would be a very small improvement though, since that way you wouldn't see the "Dash" title at all, so let me add it in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change made in c00fe4e.

path=page.path if order else "/",
order=order,
layout=partial(self._make_page_layout, page),
)
dash.register_page(module=MODULE_PAGE_404, layout=self._make_page_404_layout())

Expand Down
8 changes: 0 additions & 8 deletions vizro-core/tests/unit/vizro/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,3 @@ def vizro_app():
app instantiation.pages.
"""
return Vizro()

antonymilne marked this conversation as resolved.
Show resolved Hide resolved

@pytest.fixture()
def prebuilt_two_page_dashboard(vizro_app, page_1, page_2):
"""Minimal two page dashboard, used mainly for testing navigation."""
dashboard = vm.Dashboard(pages=[page_1, page_2])
dashboard.pre_build()
return dashboard
9 changes: 9 additions & 0 deletions vizro-core/tests/unit/vizro/models/_navigation/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import pytest

import vizro.models as vm


@pytest.fixture()
def pages_as_list():
Expand All @@ -11,3 +13,10 @@ def pages_as_list():
@pytest.fixture
def pages_as_dict():
return {"Group": ["Page 1", "Page 2"]}


@pytest.fixture()
def prebuilt_two_page_dashboard(vizro_app, page_1, page_2):
dashboard = vm.Dashboard(pages=[page_1, page_2])
dashboard.pre_build()
return dashboard
182 changes: 81 additions & 101 deletions vizro-core/tests/unit/vizro/models/test_dashboard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import json
from collections import OrderedDict
from functools import partial

import dash
import dash_bootstrap_components as dbc
import plotly
import pytest
from asserts import assert_component_equal
from dash import html

try:
Expand All @@ -18,88 +17,6 @@
from vizro.actions._action_loop._action_loop import ActionLoop


@pytest.fixture()
def dashboard_container():
return dbc.Container(
id="dashboard_container_outer",
children=[
html.Div(vizro.__version__, id="vizro_version", hidden=True),
ActionLoop._create_app_callbacks(),
dash.page_container,
],
className="vizro_dark",
fluid=True,
)


@pytest.fixture()
def mock_page_registry(prebuilt_two_page_dashboard, page_1, page_2):
return OrderedDict(
{
"Page 1": {
"module": "Page 1",
"supplied_path": "/",
"path_template": None,
"path": "/",
"supplied_name": "Page 1",
"name": "Page 1",
"supplied_title": None,
"title": "Page 1",
"description": "",
"order": 0,
"supplied_order": 0,
"supplied_layout": partial(prebuilt_two_page_dashboard._make_page_layout, page_1),
"supplied_image": None,
"image": None,
"image_url": None,
"redirect_from": None,
"layout": partial(prebuilt_two_page_dashboard._make_page_layout, page_1),
"relative_path": "/",
},
"Page 2": {
"module": "Page 2",
"supplied_path": "/page-2",
"path_template": None,
"path": "/page-2",
"supplied_name": "Page 2",
"name": "Page 2",
"supplied_title": None,
"title": "Page 2",
"description": "",
"order": 1,
"supplied_order": 1,
"supplied_layout": partial(prebuilt_two_page_dashboard._make_page_layout, page_2),
"supplied_image": None,
"image": None,
"image_url": None,
"redirect_from": None,
"layout": partial(prebuilt_two_page_dashboard._make_page_layout, page_2),
"relative_path": "/page-2",
},
"not_found_404": {
"module": "not_found_404",
"supplied_path": None,
"path_template": None,
"path": "/not-found-404",
"supplied_name": None,
"name": "Not found 404",
"supplied_title": None,
"title": "Not found 404",
"description": "",
"order": None,
"supplied_order": None,
"supplied_layout": prebuilt_two_page_dashboard._make_page_404_layout(),
"supplied_image": None,
"image": None,
"image_url": None,
"redirect_from": None,
"layout": prebuilt_two_page_dashboard._make_page_404_layout(),
"relative_path": "/not-found-404",
},
}
)


class TestDashboardInstantiation:
"""Tests model instantiation and the validators run at that time."""

Expand Down Expand Up @@ -149,27 +66,90 @@ def test_field_invalid_theme_input_type(self, page_1):
class TestDashboardPreBuild:
"""Tests dashboard pre_build method."""

def test_dashboard_page_registry(self, prebuilt_two_page_dashboard, mock_page_registry):
result = dash.page_registry
expected = mock_page_registry
# Str conversion required as comparison of OrderedDict values result in False otherwise
assert str(result.items()) == str(expected.items())
huong-li-nguyen marked this conversation as resolved.
Show resolved Hide resolved

def test_create_layout_page_404(self, prebuilt_two_page_dashboard, mocker):
mocker.patch("vizro.models._dashboard.get_relative_path")
result = prebuilt_two_page_dashboard._make_page_404_layout()
result_image = result.children[0]
result_div = result.children[1]

assert isinstance(result, html.Div)
assert isinstance(result_image, html.Img)
assert isinstance(result_div, html.Div)
def test_page_registry(self, vizro_app, page_1, page_2, mocker):
mock_register_page = mocker.patch("dash.register_page", autospec=True)
antonymilne marked this conversation as resolved.
Show resolved Hide resolved
mock_make_page_404_layout = mocker.patch(
"vizro.models._dashboard.Dashboard._make_page_404_layout"
) # Checking the actual dash components is done in test_make_page_404_layout.
vm.Dashboard(pages=[page_1, page_2]).pre_build()

mock_register_page.assert_any_call(
module=page_1.id,
name="Page 1",
title="Page 1",
path="/",
order=0,
layout=mocker.ANY, # partial call is tricky to mock out so we ignore it.
)
mock_register_page.assert_any_call(
module=page_2.id,
name="Page 2",
title="Page 2",
path="/page-2",
order=1,
layout=mocker.ANY, # partial call is tricky to mock out so we ignore it.
)
mock_register_page.assert_any_call(
module="not_found_404",
layout=mock_make_page_404_layout(),
)
assert mock_register_page.call_count == 3

def test_page_registry_with_title(self, vizro_app, page_1, mocker):
mock_register_page = mocker.patch("dash.register_page", autospec=True)
vm.Dashboard(pages=[page_1], title="My dashboard").pre_build()

mock_register_page.assert_any_call(
module=page_1.id,
name="Page 1",
title="My dashboard: Page 1",
path="/",
order=0,
layout=mocker.ANY, # partial call is tricky to mock out so we ignore it.
)
huong-li-nguyen marked this conversation as resolved.
Show resolved Hide resolved

def test_make_page_404_layout(self, vizro_app):
# vizro_app fixture is needed to avoid mocking out get_relative_path.
expected = html.Div(
[
html.Img(src="/vizro/images/errors/error_404.svg"),
html.Div(
[
html.Div(
[
html.H3("This page could not be found.", className="heading-3-600"),
html.P("Make sure the URL you entered is correct."),
],
className="error_text_container",
),
dbc.Button("Take me home", href="/", className="button_primary"),
],
className="error_content_container",
),
],
className="page_error_container",
)

assert_component_equal(vm.Dashboard._make_page_404_layout(), expected, {})


class TestDashboardBuild:
"""Tests dashboard build method."""

def test_dashboard_build(self, dashboard_container, prebuilt_two_page_dashboard):
result = json.loads(json.dumps(prebuilt_two_page_dashboard.build(), cls=plotly.utils.PlotlyJSONEncoder))
def test_dashboard_build(self, vizro_app, page_1, page_2):
dashboard = vm.Dashboard(pages=[page_1, page_2])
dashboard.pre_build()

dashboard_container = dbc.Container(
id="dashboard_container_outer",
children=[
html.Div(vizro.__version__, id="vizro_version", hidden=True),
ActionLoop._create_app_callbacks(),
dash.page_container,
],
className="vizro_dark",
fluid=True,
)
result = json.loads(json.dumps(dashboard.build(), cls=plotly.utils.PlotlyJSONEncoder))
expected = json.loads(json.dumps(dashboard_container, cls=plotly.utils.PlotlyJSONEncoder))
assert result == expected
Loading