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

Includes query parameters in rendered Link component #2464

Merged
merged 5 commits into from
Mar 7, 2022

Conversation

kuy
Copy link
Contributor

@kuy kuy commented Feb 21, 2022

Description

<Link> component of yew-router works well even if it has query parameters, but it doesn't render query parameters in href attribute of <a> tag (DOM tree). This PR resolves this issue by fixing <Link> component.

Let me explain by using router example. Here is a snippet from <Pagination> component.

html! {
    <li>
        <Link<Route, PageQuery>
            classes={classes!("pagination-link", is_current_class)}
            to={route_to_page}
            query={Some(PageQuery{page: to_page})}
        >
            { to_page }
        </Link<Route, PageQuery>>
    </li>
}

Assuming to_page variable has 2 value, the above code will be rendered...

Expected: <a href="/posts?page=2">2</a>
Actual: <a href="/posts">2</a>

Again, this is just an issue on DOM representation, not behavior.

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests
  • If this is a bug fix, these tests will fail if the bug is present

@github-actions
Copy link

github-actions bot commented Feb 21, 2022

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

https://yew-rs-api--pr2464-fix-yew-router-link-14wtmzvq.web.app

(expires Mon, 28 Feb 2022 15:07:57 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@kuy kuy force-pushed the fix-yew-router-link branch from fbd2d0f to 8336b32 Compare February 21, 2022 14:44
@kuy kuy force-pushed the fix-yew-router-link branch from 8336b32 to 8c17b85 Compare February 21, 2022 15:02
@kuy kuy marked this pull request as ready for review February 21, 2022 15:09
@kuy
Copy link
Contributor Author

kuy commented Feb 28, 2022

Could you someone start review this PR?

@kuy
Copy link
Contributor Author

kuy commented Mar 7, 2022

Hi @hamza1311 @futursolo, could you start a review of this PR? If there is a missing point, please let me know.

@futursolo
Copy link
Member

@kuy

I believe most maintainers are busy at the moment and they will eventually get to this pull request.

(BTW, your pull request is fine.)

Copy link
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

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

Sorry for the wait. Nice work and looks good to me 💯

@voidpumpkin voidpumpkin added the A-yew-router Area: The yew-router crate label Mar 7, 2022
@kuy
Copy link
Contributor Author

kuy commented Mar 7, 2022

@voidpumpkin Thanks! No problem, I know you're busy. Should I wait response from @hamza1311 or ask other maintainers? (PR requires 2 reviewers)

@futursolo Thanks! But I misunderstood that you're a maintainer of Yew because I saw your name many times in this repo...

@voidpumpkin
Copy link
Member

@kuy Currently mostly only me and @hamza1311 review Pr's. @siku2 sometimes hops in to help but not that often.
So yeah lets wait until @hamza1311 approves this PR and merges it in.

@ranile ranile merged commit e2b91ca into yewstack:master Mar 7, 2022
@kuy kuy deleted the fix-yew-router-link branch March 7, 2022 14:00
@kuy
Copy link
Contributor Author

kuy commented Mar 7, 2022

Thanks @hamza1311 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-router Area: The yew-router crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants