Enable highlighting of active page button and refactor accordion.build()

[nadijagraca]

Description

• Visual fixes:
  • Enable highlighting of active accordion button by providing page_id
  • Change the collapsible default behavior of the accordion to always be open
  • Align accordion title and page title
  • Remove highlighting of active accordion group title
• Optimise for-loop for the creation of the AccordionButtons to loop through the provided pages and then check if it exists in the dash page registry, required:
  • Move the creation of dash.page_registry to Dashboard.pre_build
  • Update _validate_pages validator to take registered pages from ModelManager as dash.page_registry is not available at point of validation
• Remove Accordion().build() from Page.build(), required:
  • Add validator for dashboard.navigation to default to Navigation()
  • Update all unit tests respectively

Screenshot

Checklist

• 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
• I have updated the docstring of any public function/class/model changed
• I have added the PR number to the change description in the changelog fragment, e.g. Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1)) (if applicable)
• I have added tests to cover my changes (if applicable)

Types of changes

• Docs/refactoring (non-breaking change which improves codebase)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

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 authorised to submit this contribution on behalf of the original creator(s) or their licensees.
  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

[huong-li-nguyen]

Looks great to me in general! Thanks for fixing all the CSS 🚀

Would align with @antonymilne regarding the build argument

[petar-qb]

I have nothing to add. Looks good! 👍

[huong-li-nguyen]

I have nothing to add. Looks good! 👍

Perfect, thanks! @antonymilne had a few improvement suggestions I have implemented, so please take another look. :)

The relevant ones are:

• Optimise for-loop for the creation of the AccordionButtons to loop through the provided pages and then check if it exists in the dash page registry (previously implemented the other way around), required:
  • Move the creation of dash.page_registry to Dashboard.pre_build
  • Update _validate_pages validator to take registered pages from ModelManager as dash.page_registry is not available at point of validation
• Remove Accordion().build() from Page.build(), required:
  • Add validator for dashboard.navigation to default to Navigation()
  • Update all unit tests respectively

[huong-li-nguyen]

@antonymilne - I've implemented the changes we've discussed and I think they are an improvement to what we've had before (Navigation being dependent on Dashboard.build()).

However, this set-up still bugs me 😄 It's related to the fact that we build the navigation in the Page, and the navigation now depends on the dashboard.pre_build(). So this page.build() test currently requires a pre-build of the dashboard and the navigation (otherwise it cannot find the private _selector attribute).

I think for now that's fine, as the pre-build steps will always be executed before the page.build(). However, I am sure we can solve this by moving the navigation.pre-build()/build() to the dashboard. Just wanted to emphasize above as it plays into our discussion enabling arguments inside the build() methods.

[antonymilne]

Agree with all this. There's actually one potentially problem which we should check: does dashboard.pre_build need to be run before navigation.pre_build in order to work? Or if you swap those round is it still ok?

Would things be neater if we moved the registration of pages into a Dashboard validator so that it's executed before any pre-build? We should think carefully before doing that though because it would be very easy to have a contaminated Dash page registry unless we empty it out first (same problem as we have with the model manager).

[huong-li-nguyen]

The order here doesn't seem to matter, the test will still pass if we switch the order. I think it's because we are re-using the same validator in the Navigation and Accordion, so either way the pages will get auto-populated if no Navigation is provided.

Given the order above doesn't matter - shall we leave as is or shall I try to move it to a validator? We actually had a problem of contaminated dash page registry leading to test interdependence which was fixed in this PR: #84

However, I had to remove each page individually as initiating with an empty OrderedDict again left the dash.page_registry permanently empty in that PR

[huong-li-nguyen]

EDIT: Seems like we can't move the registration of pages into a Dashboard validator, you'll get the error dash.exceptions.PageError: dash.register_page() must be called after app instantiation. So I guess it can only live in the pre-build or build?

[antonymilne]

Ah yes, that is a problem. Possibly we should actually treat the dash.register_page as something quite special that lives in Vizro.build itself:

    def build(self, dashboard: Dashboard):
        for page in dashboard.pages:
             dash.register_page(...)
        self._pre_build()
        ....

Let's not do it now, given that it seems to work ok in pre_build and the order doesn't seem to matter.

This does feel a bit fragile though, and I can imagine it inadvertently introducing bugs in future - we always need to stay careful to make sure that pre_build methods can run in any order with the same result, and that is easy to forget. There seem to be a few problems with page registration and model manager anyway (e.g. #59), so let's bear in mind the possibility of moving this bit to Vizro.build when thinking about those since it might improve things overall.

I have some comments/questions on #84 but let's save that for another conversation 🙂

[huong-li-nguyen]

I have a few questions on that one, but I'll add it to the list of minor topics to catch-up on! Will leave this conversation open, so we can go back :)

[petar-qb]

I like to see that the dash.page_registry moved to the Dashboard._pre_build method. It looks much cleaner now.

[antonymilne]

Part of me actually still prefers to use a clientside callback for this (like Dash pages themselves do to update the title), but I trust @petar-qb's instinct here saying that we should avoid that. I don't much like having an argument in build, but I always thought we might have to do that somewhere in our codebase eventually. So let's go for it 🙂

Possibly arguments in other build methods will help untangle other pieces like the placement of the code that currently lives in Page. But in general let's still try to avoid adding arguments in build unless it really helps a lot, like it does here.

Generally this looks great and I'm happy to approve once a couple of things have been checked! 🙏

[antonymilne]

I didn't follow all of the navigation/pages validation logic, but just to check: what happens in the case that I do this?

page_1 = Page(...)
Dashboard(pages=[Page(...)])

So page_1 exists in the model manager but not in the Dash page registry. The lookup dash.page_registry[page_id] would fail in this case, but would previous validation prevent us from hitting that error?

[huong-li-nguyen]

Ahhhh, just double-checked - the validations don't capture it. They only capture validations errors if you use pages in the Navigation that are completely unknown or missing.

It's a bit tricky - so we use this validator for the navigations: https://github.com/mckinsey/vizro/blob/vizro/bug/highlight_active_accordion_item/vizro-core/src/vizro/models/_navigation/navigation.py

The registered pages are taken from the ModelManager as at point of validation the dash.page_registry doesn't exist yet. I tried re-writing it such that it takes it from the Dashboard.pages instead of looping through all Page models in the MM, but at that point of time a model with Dashboard doesn't exist 😓

So I am currently raising a KeyError on the spot at build time, so added a try/except block here. See this commit. Do you think that's fine?

[antonymilne]

Hmm this whole navigation/page registration/model manager thing is clearly quite tricky and I'll need to spend some more time looking through the validator and rest of the code properly to understand fully.

This looks like a good solution for now, but I wonder if moving the dash.register_page even earlier in the process into Vizro.build itself will be a better solution ultimately as in my other comment 🤔 This way we wouldn't need to use the model manager to access pages at all (?), which might be a lot nicer.

[huong-li-nguyen]

I would definitely prefer that! I don't think the _validate_pages validator should use the model manager at all if we currently register all pages (whether in the dashboard or not). But I understand why Nadija wrote it that way, as I tried different approaches as mentioned above, but only the ModelManager seems to have the information at that point of time 😅

Evaluating whether we can move the dash.register_page to Vizro.build definitely makes sense to me!

Another question we can also follow up on, but is it still an option to register only pages in the model manager used in the dashboard? I'll also add it to the points for discussion

[antonymilne]

Agree with all this. There's actually one potentially problem which we should check: does dashboard.pre_build need to be run before navigation.pre_build in order to work? Or if you swap those round is it still ok?

Would things be neater if we moved the registration of pages into a Dashboard validator so that it's executed before any pre-build? We should think carefully before doing that though because it would be very easy to have a contaminated Dash page registry unless we empty it out first (same problem as we have with the model manager).

[antonymilne]

This is great, thank you for all your iteration on it! We should discuss #84 briefly and then also consider whether to do further refactoring to move the dash.register_page, but I'll need to spend a while longer getting up to speed on the existing code for that. For now I think this solution should be fine so very happy to have it merged 👍

Outside all the discussion on where to put page registration and whether build methods should take arguments I'm super happy to have the improvement to highlight the active page because this was actually my wife's first comment when I showed her the new demo also 😀