diff --git a/vizro-core/changelog.d/20230928_143738_nadija_ratkusic_graca_highlight_active_accordion_item.md b/vizro-core/changelog.d/20230928_143738_nadija_ratkusic_graca_highlight_active_accordion_item.md new file mode 100644 index 000000000..bd0ac5577 --- /dev/null +++ b/vizro-core/changelog.d/20230928_143738_nadija_ratkusic_graca_highlight_active_accordion_item.md @@ -0,0 +1,39 @@ + + + + +### Added + +- Add validator for `Dashboard.navigation` to default to `Navigation()` if not provided ([#74](https://github.com/mckinsey/vizro/pull/74)) + +### Changed + +- Move creation of `dash.page_registry` to `Dashboard.pre_build` ([#74](https://github.com/mckinsey/vizro/pull/74)) +- Change the default collapsible behavior and highlighting color of the selected accordion group ([#74](https://github.com/mckinsey/vizro/pull/74)) + + + +### Fixed + +- Add highlighting to accordion button of active page ([#74](https://github.com/mckinsey/vizro/pull/74)) + + diff --git a/vizro-core/src/vizro/models/_dashboard.py b/vizro-core/src/vizro/models/_dashboard.py index 4f88c3b49..c7c709d33 100644 --- a/vizro-core/src/vizro/models/_dashboard.py +++ b/vizro-core/src/vizro/models/_dashboard.py @@ -12,11 +12,11 @@ import vizro from vizro._constants import MODULE_PAGE_404, STATIC_URL_PREFIX from vizro.actions._action_loop._action_loop import ActionLoop -from vizro.models import VizroBaseModel +from vizro.models import Navigation, VizroBaseModel from vizro.models._models_utils import _log_call if TYPE_CHECKING: - from vizro.models import Navigation, Page + from vizro.models import Page logger = logging.getLogger(__name__) @@ -66,11 +66,19 @@ class Dashboard(VizroBaseModel): title: Optional[str] = Field(None, description="Dashboard title to appear on every page on top left-side.") @validator("pages", always=True) - def validate_pages_empty_list(cls, pages): + def validate_pages(cls, pages): if not pages: raise ValueError("Ensure this value has at least 1 item.") return pages + @validator("navigation", always=True) + def validate_navigation(cls, navigation, values): + if "pages" not in values: + return navigation + if navigation is None: + return Navigation(pages=[page.id for page in values["pages"]]) + return navigation + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) # pio is the backend global state and shouldn't be changed while @@ -79,16 +87,20 @@ def __init__(self, *args, **kwargs): pio.templates.default = self.theme @_log_call - def build(self): + def pre_build(self): # Setting order here ensures that the pages in dash.page_registry preserves the order of the List[Page]. # 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=page.build) + self._create_error_page_404() + + @_log_call + def build(self): + for page in self.pages: page.build() # TODO: ideally remove, but necessary to register slider callbacks self._update_theme() - self._create_error_page_404() return dbc.Container( id="dashboard_container", diff --git a/vizro-core/src/vizro/models/_navigation/_accordion.py b/vizro-core/src/vizro/models/_navigation/_accordion.py index bbca9cce2..b2d0c7418 100644 --- a/vizro-core/src/vizro/models/_navigation/_accordion.py +++ b/vizro-core/src/vizro/models/_navigation/_accordion.py @@ -26,24 +26,29 @@ class Accordion(VizroBaseModel): _validate_pages = validator("pages", allow_reuse=True, always=True)(_validate_pages) @_log_call - def build(self): - return self._create_accordion() + def build(self, *, active_page_id=None): + return self._create_accordion(active_page_id=active_page_id) - def _create_accordion_buttons(self, pages): - """Creates a button for each provided page.""" - # TODO: Better if we loop through pages from MM so the Accordion.build does not depend on dashboard build. - # However, this would require that only pages used in the Dashboard are registered in the MM. - # Note: Relative path currently deviates from page.path for first page. - return [ - dbc.Button( - children=[page["name"]], - key=page["relative_path"], - className="accordion-item-button", - href=page["relative_path"], + def _create_accordion_buttons(self, pages, active_page_id): + """Creates a button for each provided page that is registered.""" + accordion_buttons = [] + for page_id in pages: + try: + page = dash.page_registry[page_id] + except KeyError as exc: + raise KeyError( + f"Page with ID {page_id} cannot be found. Please add the page to `Dashboard.pages`" + ) from exc + accordion_buttons.append( + dbc.Button( + children=[page["name"]], + key=page["relative_path"], + className="accordion-item-button", + active=page_id == active_page_id, + href=page["relative_path"], + ) ) - for page in dash.page_registry.values() - if page["module"] in pages - ] + return accordion_buttons def _create_accordion_item(self, accordion_buttons, title=ACCORDION_DEFAULT_TITLE): """Creates an accordion item for each sub-group of pages.""" @@ -66,6 +71,7 @@ def _get_accordion_container(self, accordion_items, accordion_buttons): class_name="accordion", persistence=True, persistence_type="session", + always_open=True, ), html.Hr(), ], @@ -73,17 +79,17 @@ def _get_accordion_container(self, accordion_items, accordion_buttons): id=f"{self.id}_outer", ) - def _create_accordion(self): + def _create_accordion(self, active_page_id): """Creates a custom accordion only with user-provided pages.""" accordion_items = [] if isinstance(self.pages, dict): for page_group, page_members in self.pages.items(): - accordion_buttons = self._create_accordion_buttons(pages=page_members) + accordion_buttons = self._create_accordion_buttons(pages=page_members, active_page_id=active_page_id) accordion_items.append( self._create_accordion_item(accordion_buttons=accordion_buttons, title=page_group) ) if isinstance(self.pages, list): - accordion_buttons = self._create_accordion_buttons(pages=self.pages) + accordion_buttons = self._create_accordion_buttons(pages=self.pages, active_page_id=active_page_id) accordion_items.append(self._create_accordion_item(accordion_buttons=accordion_buttons)) return self._get_accordion_container(accordion_items=accordion_items, accordion_buttons=accordion_buttons) diff --git a/vizro-core/src/vizro/models/_navigation/navigation.py b/vizro-core/src/vizro/models/_navigation/navigation.py index 1bf765493..26fb9cffd 100644 --- a/vizro-core/src/vizro/models/_navigation/navigation.py +++ b/vizro-core/src/vizro/models/_navigation/navigation.py @@ -3,10 +3,8 @@ import warnings from typing import TYPE_CHECKING, Optional -import dash from pydantic import PrivateAttr, validator -from vizro._constants import MODULE_PAGE_404 from vizro.managers import model_manager from vizro.models import VizroBaseModel from vizro.models._models_utils import _log_call @@ -20,36 +18,34 @@ def _validate_pages(pages): from vizro.models import Page - if pages is not None and not pages: - raise ValueError("Ensure this value has at least 1 item.") - - if pages: - registered_pages = [page[0] for page in model_manager._items_with_type(Page)] - - if isinstance(pages, dict): - missing_pages = [ - page - for page in registered_pages - if page not in {page for nav_pages in pages.values() for page in nav_pages} - ] - unknown_pages = [page for nav_pages in pages.values() for page in nav_pages if page not in registered_pages] - else: - missing_pages = [page for page in registered_pages if page not in pages] - unknown_pages = [page for page in pages if page not in registered_pages] - - if missing_pages: - warnings.warn( - f"Not all registered pages used in Navigation 'pages'. Missing pages {missing_pages}!", UserWarning - ) - - if unknown_pages: - raise ValueError( - f"Unknown page ID or page title provided to Navigation 'pages'. " f"Unknown pages: {unknown_pages}" - ) + registered_pages = [page[0] for page in model_manager._items_with_type(Page)] if pages is None: - return [page for page in dash.page_registry.keys() if page != MODULE_PAGE_404] + return registered_pages + + if not pages: + raise ValueError("Ensure this value has at least 1 item.") + if isinstance(pages, dict): + missing_pages = [ + page + for page in registered_pages + if page not in {page for nav_pages in pages.values() for page in nav_pages} + ] + unknown_pages = [page for nav_pages in pages.values() for page in nav_pages if page not in registered_pages] + else: + missing_pages = [page for page in registered_pages if page not in pages] + unknown_pages = [page for page in pages if page not in registered_pages] + + if missing_pages: + warnings.warn( + f"Not all registered pages used in Navigation 'pages'. Missing pages {missing_pages}!", UserWarning + ) + + if unknown_pages: + raise ValueError( + f"Unknown page ID or page title provided to Navigation 'pages'. " f"Unknown pages: {unknown_pages}" + ) return pages @@ -77,5 +73,5 @@ def _set_selector(self): self._selector = Accordion(pages=self.pages) @_log_call - def build(self): - return self._selector.build() + def build(self, *, active_page_id=None): + return self._selector.build(active_page_id=active_page_id) diff --git a/vizro-core/src/vizro/models/_page.py b/vizro-core/src/vizro/models/_page.py index 192a4f0b8..93e8a08fc 100644 --- a/vizro-core/src/vizro/models/_page.py +++ b/vizro-core/src/vizro/models/_page.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import List, Optional +from typing import List, Optional, cast import dash_bootstrap_components as dbc import dash_daq as daq @@ -12,7 +12,7 @@ from vizro.actions import _on_page_load from vizro.managers import model_manager from vizro.managers._model_manager import DuplicateIDError -from vizro.models import Action, Dashboard, Graph, Layout, VizroBaseModel +from vizro.models import Action, Dashboard, Graph, Layout, Navigation, VizroBaseModel from vizro.models._action._actions_chain import ActionsChain, Trigger from vizro.models._models_utils import _log_call, get_unique_grid_component_ids @@ -167,13 +167,8 @@ def _create_control_panel(controls_content): return control_panel if controls_content else None def _create_nav_panel(self): - from vizro.models._navigation._accordion import Accordion - _, dashboard = next(model_manager._items_with_type(Dashboard)) - if dashboard.navigation: - return dashboard.navigation.build() - - return Accordion().build() + return cast(Navigation, dashboard.navigation).build(active_page_id=self.id) def _create_component_container(self, components_content): component_container = html.Div( diff --git a/vizro-core/src/vizro/static/css/accordion.css b/vizro-core/src/vizro/static/css/accordion.css index 1ed9eb28a..5aa55f306 100644 --- a/vizro-core/src/vizro/static/css/accordion.css +++ b/vizro-core/src/vizro/static/css/accordion.css @@ -48,7 +48,6 @@ .accordion-button:not(.collapsed) { color: var(--text-primary); - background-color: var(--state-overlays-selected); } .accordion-button:not(.collapsed)::after { @@ -113,6 +112,11 @@ div.page_container .accordion-item-button { line-height: var(--text-size-03); } +.accordion-item-button.btn.btn-primary.active { + background-color: var(--state-overlays-selected); + color: var(--text-primary); +} + /* * Since the text inside a accordion btn is very specific * Adding important to increase the specificity diff --git a/vizro-core/src/vizro/static/css/layout.css b/vizro-core/src/vizro/static/css/layout.css index 4edf9f029..dc84572a5 100644 --- a/vizro-core/src/vizro/static/css/layout.css +++ b/vizro-core/src/vizro/static/css/layout.css @@ -11,7 +11,7 @@ display: flex; flex: 0 0 auto; flex-direction: column; - padding: 40px 32px 0 32px; + padding: 30px 32px 0 32px; scrollbar-gutter: stable; width: 352px; overflow: auto; diff --git a/vizro-core/tests/unit/vizro/conftest.py b/vizro-core/tests/unit/vizro/conftest.py index f9a55eae5..c9cacf32e 100644 --- a/vizro-core/tests/unit/vizro/conftest.py +++ b/vizro-core/tests/unit/vizro/conftest.py @@ -42,7 +42,7 @@ def dashboard(page1, page2): @pytest.fixture() -def dashboard_build(dashboard): - yield dashboard.build() +def dashboard_prebuild(dashboard): + yield dashboard.pre_build() del dash.page_registry["Page 1"] del dash.page_registry["Page 2"] diff --git a/vizro-core/tests/unit/vizro/models/_navigation/conftest.py b/vizro-core/tests/unit/vizro/models/_navigation/conftest.py index 754fe8eff..7bf6e428d 100644 --- a/vizro-core/tests/unit/vizro/models/_navigation/conftest.py +++ b/vizro-core/tests/unit/vizro/models/_navigation/conftest.py @@ -23,12 +23,14 @@ def accordion_from_page_as_list(): dbc.Button( children=["Page 1"], className="accordion-item-button", + active=True, key="/", href="/", ), dbc.Button( children=["Page 2"], className="accordion-item-button", + active=False, key="/page-2", href="/page-2", ), @@ -48,6 +50,7 @@ def accordion_from_page_as_list(): class_name="accordion", persistence=True, persistence_type="session", + always_open=True, ), html.Hr(), ], @@ -61,13 +64,17 @@ def accordion_from_page_as_list(): def accordion_from_pages_as_dict(): accordion_items = [ dbc.AccordionItem( - children=[dbc.Button(children=["Page 1"], className="accordion-item-button", key="/", href="/")], + children=[ + dbc.Button(children=["Page 1"], className="accordion-item-button", active=True, key="/", href="/") + ], title="PAGE 1", class_name="accordion_item", ), dbc.AccordionItem( children=[ - dbc.Button(children=["Page 2"], className="accordion-item-button", key="/page-2", href="/page-2") + dbc.Button( + children=["Page 2"], className="accordion-item-button", active=False, key="/page-2", href="/page-2" + ) ], title="PAGE 2", class_name="accordion_item", @@ -81,6 +88,7 @@ def accordion_from_pages_as_dict(): class_name="accordion", persistence=True, persistence_type="session", + always_open=True, ), html.Hr(), ], diff --git a/vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py b/vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py index e5d3070d5..b60575485 100644 --- a/vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py +++ b/vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py @@ -9,7 +9,7 @@ from vizro.models._navigation._accordion import Accordion -@pytest.mark.usefixtures("dashboard_build") +@pytest.mark.usefixtures("dashboard_prebuild") class TestAccordionInstantiation: """Tests accordion model instantiation.""" @@ -37,25 +37,25 @@ def test_field_invalid_pages_empty_list(self): Accordion(pages=[]) -@pytest.mark.usefixtures("dashboard_build") +@pytest.mark.usefixtures("dashboard_prebuild") class TestAccordionBuild: """Tests accordion build method.""" @pytest.mark.parametrize("pages", [["Page 1", "Page 2"], None]) def test_accordion_build_default(self, pages, accordion_from_page_as_list): - accordion = Accordion(pages=pages, id="accordion_list").build() + accordion = Accordion(pages=pages, id="accordion_list").build(active_page_id="Page 1") result = json.loads(json.dumps(accordion, cls=plotly.utils.PlotlyJSONEncoder)) expected = json.loads(json.dumps(accordion_from_page_as_list, cls=plotly.utils.PlotlyJSONEncoder)) assert result == expected def test_accordion_build_pages_as_list(self, pages_as_list, accordion_from_page_as_list): - accordion = Accordion(pages=pages_as_list, id="accordion_list").build() + accordion = Accordion(pages=pages_as_list, id="accordion_list").build(active_page_id="Page 1") result = json.loads(json.dumps(accordion, cls=plotly.utils.PlotlyJSONEncoder)) expected = json.loads(json.dumps(accordion_from_page_as_list, cls=plotly.utils.PlotlyJSONEncoder)) assert result == expected def test_accordion_build_pages_as_dict(self, pages_as_dict, accordion_from_pages_as_dict): - accordion = Accordion(pages=pages_as_dict, id="accordion_dict").build() + accordion = Accordion(pages=pages_as_dict, id="accordion_dict").build(active_page_id="Page 1") result = json.loads(json.dumps(accordion, cls=plotly.utils.PlotlyJSONEncoder)) expected = json.loads(json.dumps(accordion_from_pages_as_dict, cls=plotly.utils.PlotlyJSONEncoder)) assert result == expected diff --git a/vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py b/vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py index 4c1b7f932..2f3590224 100644 --- a/vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py +++ b/vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py @@ -9,7 +9,7 @@ from vizro.models._navigation._accordion import Accordion -@pytest.mark.usefixtures("dashboard_build") +@pytest.mark.usefixtures("dashboard_prebuild") class TestNavigationInstantiation: """Tests navigation model instantiation.""" @@ -45,7 +45,7 @@ def test_field_invalid_pages_input(self, pages, expected_error): vm.Navigation(pages=pages) -@pytest.mark.usefixtures("dashboard_build") +@pytest.mark.usefixtures("dashboard_prebuild") class TestNavigationBuild: """Tests navigation build method.""" diff --git a/vizro-core/tests/unit/vizro/models/test_dashboard.py b/vizro-core/tests/unit/vizro/models/test_dashboard.py index 5d46680a5..54afe04c6 100644 --- a/vizro-core/tests/unit/vizro/models/test_dashboard.py +++ b/vizro-core/tests/unit/vizro/models/test_dashboard.py @@ -130,32 +130,37 @@ def test_field_invalid_theme_input_type(self, page1): vm.Dashboard(pages=[page1], theme="not_existing") -class TestDashboardBuild: - """Tests dashboard build method.""" - - def test_dashboard_build(self, dashboard_container, dashboard_build): - 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 +class TestDashboardPreBuild: + """Tests dashboard pre_build method.""" - def test_dashboard_page_registry(self, mock_page_registry, dashboard_build): + def test_dashboard_page_registry(self, mock_page_registry, dashboard_prebuild): 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()) + def test_create_layout_page_404(self): + result = create_layout_page_404() + 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) + + +class TestDashboardBuild: + """Tests dashboard build method.""" + + def test_dashboard_build(self, dashboard_container, dashboard): + dashboard.pre_build() + dashboard.navigation.pre_build() + 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 + @pytest.mark.parametrize("on, expected", [(True, "vizro_dark"), (False, "vizro_light")]) def test_update_theme(on, expected): result = update_theme(on) assert result == expected - - -def test_create_layout_page_404(): - result = create_layout_page_404() - 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) diff --git a/vizro-core/tests/unit/vizro/models/test_page.py b/vizro-core/tests/unit/vizro/models/test_page.py index 6505eff57..0eadd58dd 100644 --- a/vizro-core/tests/unit/vizro/models/test_page.py +++ b/vizro-core/tests/unit/vizro/models/test_page.py @@ -101,6 +101,8 @@ def test_action_auto_generation_valid(self, standard_px_chart): # TODO: Add unit tests for private methods in page build def test_page_build_left_side_removed(standard_px_chart): page = vm.Page(title="Single Page", components=[vm.Graph(id="scatter_chart", figure=standard_px_chart)]) - vm.Dashboard(pages=[page]).build() - assert ("className='left_side'" not in str(page.build())) is True + dashboard = vm.Dashboard(pages=[page]) + dashboard.pre_build() + dashboard.navigation.pre_build() + assert "className='left_side'" not in str(page.build()) del dash.page_registry["Single Page"]