Refactor Navigation and Accordion

[huong-li-nguyen]

Description

• Refactor Navigation and Accordion and update unit tests accordingly

User relevant changes:

• Remove user warning if there are missing pages (not all registered pages added to navigation)
• Auto-populate navigation.pages if None

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 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.

[antonymilne]

Looks great, thank you very much for finishing this off! ⭐

Just one tip for PR hygiene: the changes to _validate_pages are quite hard to review because the code was changed and it moved file. Where possible (tbh often it's not unless you know what you're doing in advance or are happy rewriting git history) it's nice to do these commits in a fixed order:

1. Make all changes to _validate_pages
2. Then move it to a different file

The opposite order is also fine, but doing 1 then 2 then 1 again is not so good. The idea is that the reviewer should be able to see what are all the actual functional changes by looking at the diff between two commits that have the code in the same file rather than needing to compare between two different files.

Doesn't make any difference here since it's a pretty small function and I know what the changes are anyway, but just a general tip 🙁

[maxschulz-COL]

Before taking a look, can you add a little bit of background to the PR why the refactoring was necessary? What didn't work before, and what is better now?

[huong-li-nguyen]

Before taking a look, can you add a little bit of background to the PR why the refactoring was necessary? What didn't work before, and what is better now?

There was room for clean-up and improvement and we had the capacity to do so, so there wasn't a necessity in terms of having to fix something. The new code works as before, so functionally nothing has changed except for the two minor changes I've added to the changelog. It's just a nice to have :)

Trying to remember everything - feel free to add @antonymilne :

• Extended validators to auto-populate dashboard.navigation for the case if dashboard.navigation = Navigation()
• Initiated type conversion (from list to dict) early on (in validator instead of Accordion.build) so we don't have to deal with the different types later on
• Removed shallow functions and just directly copied in the code in Accordion.build
• Aligned that build methods should always return a dash object, so instead of None we return a hidden html.Div in the Accordion.build
• Moved the reusable validator to a separate utils file

[huong-li-nguyen]

Looks great, thank you very much for finishing this off! ⭐

Just one tip for PR hygiene: the changes to _validate_pages are quite hard to review because the code was changed and it moved file. Where possible (tbh often it's not unless you know what you're doing in advance or are happy rewriting git history) it's nice to do these commits in a fixed order:

1. Make all changes to _validate_pages
2. Then move it to a different file

The opposite order is also fine, but doing 1 then 2 then 1 again is not so good. The idea is that the reviewer should be able to see what are all the actual functional changes by looking at the diff between two commits that have the code in the same file rather than needing to compare between two different files.

Doesn't make any difference here since it's a pretty small function and I know what the changes are anyway, but just a general tip 🙁

Makes a lot of sense, will take into account in the future 👍

[maxschulz-COL]

LGTM! Left a few comments, mainly for my understanding. Forgive if there are silly questions that I should have understood!

[huong-li-nguyen]

LGTM! Left a few comments, mainly for my understanding. Forgive if there are silly questions that I should have understood!

No worries ❤️ I'll let you resolve the open threads yourself if it's clear to you, otherwise let's discuss :)