Refactor Navigation and Accordion (#117)

Co-authored-by: Antony Milne <[email protected]>

vizro-core/changelog.d/20231017_212005_huong_li_nguyen_navigation.md

@@ -0,0 +1,39 @@
+<!--
+A new scriv changelog fragment.
+
+Uncomment the section that is right (remove the HTML comment wrapper).
+-->
+
+### Removed
+
+- Remove warning message if not all registered pages are used in `Navigation` ([#117](https://github.com/mckinsey/vizro/pull/117))
+
+<!--
+### Added
+
+- A bullet item for the Added category.
+
+-->
+
+### Changed
+
+- Autopopulate `navigation.pages` with registered pages during `Dashboard` validation if `navigation.pages = None` ([#117](https://github.com/mckinsey/vizro/pull/117))
+
+<!--
+### Deprecated
+
+- A bullet item for the Deprecated category.
+
+-->
+<!--
+### Fixed
+
+- A bullet item for the Fixed category.
+
+-->
+<!--
+### Security
+
+- A bullet item for the Security category.
+
+-->

vizro-core/schemas/0.1.5.dev0.json

@@ -859,7 +859,7 @@
     },
     "Navigation": {
       "title": "Navigation",
-      "description": "Navigation in [`Dashboard`][vizro.models.Dashboard] to structure [`Pages`][vizro.models.Page].\n\nArgs:\n    pages (Optional[NavigationPagesType]): See [NavigationPagesType][vizro.models.types.NavigationPagesType].\n        Defaults to `None`.",
+      "description": "Navigation in [`Dashboard`][vizro.models.Dashboard] to structure [`Pages`][vizro.models.Page].\n\nArgs:\n    pages (Optional[NavigationPagesType]): See [`NavigationPagesType`][vizro.models.types.NavigationPagesType].\n        Defaults to `None`.",
       "type": "object",
       "properties": {
         "id": {

vizro-core/src/vizro/models/_dashboard.py

@@ -75,7 +75,8 @@ def validate_pages(cls, pages):
     def validate_navigation(cls, navigation, values):
         if "pages" not in values:
             return navigation
-        if navigation is None:
+
+        if navigation is None or not navigation.pages:
             return Navigation(pages=[page.id for page in values["pages"]])
         return navigation
 

vizro-core/src/vizro/models/_navigation/_accordion.py

@@ -1,67 +1,53 @@
-from typing import Optional
+import itertools
+from collections.abc import Mapping
+from typing import Dict, List
 
 import dash
 import dash_bootstrap_components as dbc
 from dash import html
-from pydantic import validator
+from pydantic import Field, validator
 
 from vizro._constants import ACCORDION_DEFAULT_TITLE
 from vizro.models import VizroBaseModel
 from vizro.models._models_utils import _log_call
-from vizro.models._navigation.navigation import _validate_pages
-from vizro.models.types import NavigationPagesType
+from vizro.models._navigation._navigation_utils import _validate_pages
 
 
 class Accordion(VizroBaseModel):
-    """Accordion to be used in Navigation Panel of Dashboard.
+    """Accordion to be used as selector in [`Navigation`][vizro.models.Navigation].
 
     Args:
-        pages (Optional[NavigationPagesType]): See [NavigationPagesType][vizro.models.types.NavigationPagesType].
-            Defaults to `None`.
+        pages (Dict[str, List[str]]): A dictionary with a page group title as key and a list of page IDs as values.
     """
 
-    pages: Optional[NavigationPagesType] = None
+    pages: Dict[str, List[str]] = Field(
+        ..., description="A dictionary with a page group title as key and a list of page IDs as values."
+    )
 
-    # validators
-    _validate_pages = validator("pages", allow_reuse=True, always=True)(_validate_pages)
+    _validate_pages = validator("pages", allow_reuse=True)(_validate_pages)
+
+    @validator("pages", pre=True)
+    def coerce_pages_type(cls, pages):
+        if isinstance(pages, Mapping):
+            return pages
+        return {ACCORDION_DEFAULT_TITLE: pages}
 
     @_log_call
     def build(self, *, active_page_id=None):
-        return self._create_accordion(active_page_id=active_page_id)
+        # Hide navigation panel if there is only one page
+        if len(list(itertools.chain(*self.pages.values()))) == 1:
+            return html.Div(className="hidden")
 
-    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"],
+        accordion_items = []
+        for page_group, page_members in self.pages.items():
+            accordion_buttons = self._create_accordion_buttons(pages=page_members, active_page_id=active_page_id)
+            accordion_items.append(
+                dbc.AccordionItem(
+                    children=accordion_buttons,
+                    title=page_group.upper(),
+                    class_name="accordion_item",
                 )
             )
-        return accordion_buttons
-
-    def _create_accordion_item(self, accordion_buttons, title=ACCORDION_DEFAULT_TITLE):
-        """Creates an accordion item for each sub-group of pages."""
-        return dbc.AccordionItem(
-            children=accordion_buttons,
-            title=title.upper(),
-            class_name="accordion_item",
-        )
-
-    def _get_accordion_container(self, accordion_items, accordion_buttons):
-        # Return no container if there is only one page in the dashboard or no pages exist
-        if (len(accordion_buttons) == len(accordion_items) == 1) or not accordion_buttons:
-            return None
 
         return html.Div(
             children=[
@@ -79,17 +65,23 @@ def _get_accordion_container(self, accordion_items, accordion_buttons):
             id="nav_panel_outer",
         )
 
-    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, active_page_id=active_page_id)
-                accordion_items.append(
-                    self._create_accordion_item(accordion_buttons=accordion_buttons, title=page_group)
+    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"],
                 )
-
-        if isinstance(self.pages, list):
-            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)
+            )
+        return accordion_buttons

vizro-core/src/vizro/models/_navigation/_navigation_utils.py

@@ -0,0 +1,21 @@
+import itertools
+
+from vizro.managers import model_manager
+
+
+def _validate_pages(pages):
+    """Reusable validator to check if provided Page IDs exist as registered pages."""
+    from vizro.models import Page
+
+    pages_as_list = list(itertools.chain(*pages.values())) if isinstance(pages, dict) else pages
+
+    if not pages_as_list:
+        raise ValueError("Ensure this value has at least 1 item.")
+
+    # Ideally we would use dash.page_registry or maybe dashboard.pages here, but we only register pages in
+    # dashboard.pre_build and model manager cannot find a Dashboard at validation time.
+    # page[0] gives the page model ID.
+    registered_pages = [page[0] for page in model_manager._items_with_type(Page)]
+    if unknown_pages := [page for page in pages_as_list if page not in registered_pages]:
+        raise ValueError(f"Unknown page ID {unknown_pages} provided to argument 'pages'.")
+    return pages

vizro-core/src/vizro/models/_navigation/navigation.py

@@ -1,76 +1,37 @@
 from __future__ import annotations
 
-import warnings
 from typing import TYPE_CHECKING, Optional
 
 from pydantic import PrivateAttr, validator
 
-from vizro.managers import model_manager
 from vizro.models import VizroBaseModel
 from vizro.models._models_utils import _log_call
+from vizro.models._navigation._navigation_utils import _validate_pages
 from vizro.models.types import NavigationPagesType
 
 if TYPE_CHECKING:
     from vizro.models._navigation._accordion import Accordion
 
 
-# Validator for reuse in other models to validate pages
-def _validate_pages(pages):
-    from vizro.models import Page
-
-    registered_pages = [page[0] for page in model_manager._items_with_type(Page)]
-
-    if pages is None:
-        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
-
-
 class Navigation(VizroBaseModel):
     """Navigation in [`Dashboard`][vizro.models.Dashboard] to structure [`Pages`][vizro.models.Page].
 
     Args:
-        pages (Optional[NavigationPagesType]): See [NavigationPagesType][vizro.models.types.NavigationPagesType].
+        pages (Optional[NavigationPagesType]): See [`NavigationPagesType`][vizro.models.types.NavigationPagesType].
             Defaults to `None`.
     """
 
     pages: Optional[NavigationPagesType] = None
     _selector: Accordion = PrivateAttr()
 
     # validators
-    _validate_pages = validator("pages", allow_reuse=True, always=True)(_validate_pages)
+    _validate_pages = validator("pages", allow_reuse=True)(_validate_pages)
 
     @_log_call
     def pre_build(self):
-        self._set_selector()
-
-    def _set_selector(self):
         from vizro.models._navigation._accordion import Accordion
 
-        self._selector = Accordion(pages=self.pages)
+        self._selector = Accordion(pages=self.pages)  # type: ignore[arg-type]
 
     @_log_call
     def build(self, *, active_page_id=None):

vizro-core/src/vizro/static/css/layout.css

@@ -91,3 +91,7 @@
   flex-direction: column;
   width: 100%;
 }
+
+.hidden {
+  display: none;
+}

vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py

@@ -3,6 +3,7 @@
 
 import plotly
 import pytest
+from dash import html
 from pydantic import ValidationError
 
 import vizro.models as vm
@@ -13,41 +14,39 @@
 class TestAccordionInstantiation:
     """Tests accordion model instantiation."""
 
-    def test_create_accordion_default(self):
-        accordion = Accordion(id="accordion_id")
-        assert accordion.id == "accordion_id"
-        assert accordion.pages == ["Page 1", "Page 2"]
-
-    def test_create_accordion_pages_as_list(self, pages_as_list):
+    def test_accordion_valid_pages_as_list(self, pages_as_list):
         accordion = Accordion(pages=pages_as_list, id="accordion_id")
         assert accordion.id == "accordion_id"
-        assert accordion.pages == pages_as_list
+        assert accordion.pages == {"SELECT PAGE": pages_as_list}
 
-    def test_create_accordion_pages_as_dict(self, pages_as_dict):
+    def test_accordion_valid_pages_as_dict(self, pages_as_dict):
         accordion = Accordion(pages=pages_as_dict, id="accordion_id")
         assert accordion.id == "accordion_id"
         assert accordion.pages == pages_as_dict
 
-    def test_field_invalid_pages_input_type(self):
-        with pytest.raises(ValidationError, match="2 validation errors for Accordion"):
-            Accordion(pages=[vm.Button()])
+    def test_navigation_valid_pages_not_all_included(self):
+        accordion = Accordion(pages=["Page 1"], id="accordion_id")
+        assert accordion.id == "accordion_id"
+        assert accordion.pages == {"SELECT PAGE": ["Page 1"]}
+
+    def test_invalid_field_pages_required(self):
+        with pytest.raises(ValidationError, match="field required"):
+            Accordion()
 
-    def test_field_invalid_pages_empty_list(self):
+    @pytest.mark.parametrize("pages", [{"SELECT PAGE": []}, []])
+    def test_invalid_field_pages_no_ids_provided(self, pages):
         with pytest.raises(ValidationError, match="Ensure this value has at least 1 item."):
-            Accordion(pages=[])
+            Accordion(pages=pages)
+
+    def test_invalid_field_pages_wrong_input_type(self):
+        with pytest.raises(ValidationError, match="str type expected"):
+            Accordion(pages=[vm.Page(title="Page 3", components=[vm.Button()])])
 
 
 @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(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(active_page_id="Page 1")
         result = json.loads(json.dumps(accordion, cls=plotly.utils.PlotlyJSONEncoder))
@@ -60,10 +59,8 @@ def test_accordion_build_pages_as_dict(self, pages_as_dict, accordion_from_pages
         expected = json.loads(json.dumps(accordion_from_pages_as_dict, cls=plotly.utils.PlotlyJSONEncoder))
         assert result == expected
 
-    def test_accordion_build_single_page_accordion(self):
-        accordion = Accordion(pages=["Page 1"], id="single_accordion").build()
-        assert accordion is None
-
-    def test_navigation_not_all_pages_included(self):
-        with pytest.warns(UserWarning):
-            Accordion(pages=["Page 1"])
+    def test_single_page_and_hidden_div(self):
+        accordion = Accordion(pages=["Page 1"]).build()
+        result = json.loads(json.dumps(accordion, cls=plotly.utils.PlotlyJSONEncoder))
+        expected = json.loads(json.dumps(html.Div(className="hidden"), cls=plotly.utils.PlotlyJSONEncoder))
+        assert result == expected