No validation for Card.href can break page opening

l0uden commented 6 months ago

Description

If we create path="/page page" for any page it will be available in browser like /page-page. At the same time if we create Card.href="/page page" we'll get 404 during page opening.

Looks like to synchronise this values we should apply same validation for href and path

Expected behavior

No response

Which package?

vizro

Package version

0.1.8

Python version

3.9

OS

Mac OS

How to Reproduce

Create one page with path="/page page"
Create second page with Card.href="/page page"
Run the dashboard and click Card on the second page.

Output

No response

Code of Conduct

[X] I agree to follow the Code of Conduct.

antonymilne commented 6 months ago

Currently this is the expected behaviour, but I think you're right we should change the behaviour.

There's two different things here:

when you set Page.path, it's correct that we convert whitespace to - to make a clean valid URL for the page
when you set Card.href, remember this can be a link to anything, not just a link inside your app. Hence it's valid to put in e.g. Card.href = "http://www.example.com/something with space" (space is not a valid character in a URL and it should be encoded as %20, but this encoding always happens automatically when you click the link so there's not really any point us doing it I think. Even if we did encode it, we should not convert spaces to - because it would contradict the correct HTML encoding of %20)

The case you give here is a special (but common) case where you use Card.href to refer to a page in the app and hence use a relative link. We handle this as:

href=get_relative_path(self.href) if self.href.startswith("/") else self.href

I think we should change this:

put self.href through the same clean_path transformation that Page.path has in the case that it's a relative path. Hence writing Card.href = "/page page" would then work
even better, make it so that you can just write Card.href = "page page" without the leading / and it would still work. This would mean we need a better check to see if the href is relative or absolute and to add the / back in the case that it's not absolute and it's missing

This way a user will be able to put the page title directly in the Card.href without thinking about it even if it's got weird characters, i.e. this should work:

Page(title = "something weird %*")
Card(href = "something weird %*")

antonymilne commented 4 months ago

After more thought we should not implement this feature for now. See #348 for explanation.

mckinsey / vizro