Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update nested router docs #2535

Merged
merged 4 commits into from
Mar 20, 2022
Merged

Conversation

voidpumpkin
Copy link
Member

Description

Router nested example is bugged in a way where you cant have settings/ page. My edits fix this.

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

github-actions[bot]
github-actions bot previously approved these changes Mar 20, 2022
@github-actions
Copy link

github-actions bot commented Mar 20, 2022

Visit the preview URL for this PR (updated for commit af4c39a):

https://yew-rs--pr2535-doccu-router-s42wrptb.web.app

(expires Sun, 27 Mar 2022 10:09:33 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@@ -425,8 +433,6 @@ enum MainRoute {
News,
#[at("/contact")]
Contact,
#[at("/settings/:s")]
Settings,
Copy link
Member

@futursolo futursolo Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think modifying the settings variant to the following would fix it?

#[at("/settings/*s")]
Settings {
    s: String,
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?
There was no bug in the actual code before, but users kept trying to add /settings and that does not get matched, its a bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought route_recognizer can match wildcard optionally...

In this case you can fix it with:

#[at("/settings/")]
SettingsHome,
#[at("/settings/*s")]
Settings,

and match it with:

MainRoute::SettingsHome | MainRoute::Settings => {}

This is still a less than ideal fix, but better than matching it in NotFound.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im like 80% sure SettingsHome wont match, let me try...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait you wanted SettingsHome to be in MainRoute that is bad as we want all nested routes to be in the SettingsRoute enum.
Kind of defeats the purpose if to go home of settings i cant use SettingsRoute and am forced into using parent route.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can match /settings/ again in the SettingsRoute?

MainRoute::SettingsHome | MainRoute::Settings => {
    html! { <Switch<SettingsRoute> render={Switch::render(switch_settings)} /> }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok that does work, let me update then...

@voidpumpkin voidpumpkin mentioned this pull request Mar 20, 2022
3 tasks
github-actions[bot]
github-actions bot previously approved these changes Mar 20, 2022
@voidpumpkin voidpumpkin merged commit 5ececa4 into yewstack:master Mar 20, 2022
@voidpumpkin voidpumpkin deleted the doccu-router branch March 20, 2022 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants