mckinsey / vizro

Vizro is a toolkit for creating modular data visualization applications.
https://vizro.readthedocs.io/en/stable/
Apache License 2.0
2.65k stars 136 forks source link

[Bug] Consolidate validation for `Card.href` and `Page.path` #348

Closed huong-li-nguyen closed 6 months ago

huong-li-nguyen commented 7 months ago

Description

Screenshot

Notice

huong-li-nguyen commented 6 months ago

Closing as we decided to not enable this feature due to the complexity and ambiguity it might create when users provide different page.title / page.id / page.href

antonymilne commented 6 months ago

In more detail...

The problem with the proposed solution for Card.href is that really we should be referring to the page id and not the page title. But the page id is not necessarily the same as the page path since (a) there’s discrepancy of whether / is scrubbed from the path depending on whether you set the page.path manually or whether it's taken from the id automatically (b) the annoying homepage case where id/title and path don’t match up (maybe changed in #366) and (c) we let users set page.path manually (which in hindsight was a mistake I think since I’m pretty sure no one does it now and it makes this sort of thing a bit awkward :grimacing:). So that means that page.path could be completely different from page.id, in which case the solution proposed in this PR won't work at all.

So really the way we should be doing this is:

https://github.com/mckinsey/vizro/pull/348/commits/09305d69638dd80a1369691bbd9c7a9edaa196ea shows how we might do this. While it's not that hard to do, it does add some complexity and at some point in the future I'd hope that the current href behaviour of Card might be removed at all in favour of #226.

If we do add this in future to lookup by page.id we should also consider whether to remove the functionality that providing a relative path would work so that the only options are to provide page.id or a complete href. This might be a bit tidier but would be a breaking change.