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

Mention certain components must be children of Router #2913

Merged
merged 6 commits into from
Dec 10, 2022

Conversation

sasacocic
Copy link
Contributor

Description

I've added documentation explaining that certain elements like <Link /> & <Switch /> must be nested inside of a router.

Fixes #2903

@sasacocic
Copy link
Contributor Author

sasacocic commented Oct 8, 2022

I've been using yew for a little bit now and have some ideas about improving documentation for yew. These aren't complete thoughts, but I just wanted to share what was on my mind as it might be helpful.

Also to share my experience reading the docs:

  • I will found the Tutorial to be helpful, but the actual Docs section not that helpful. Often I felt it included a lot of verboseness that deviated from just explaining the core of the concept and how to use it.

I'm not in the loop about what is being done with the docs, but my suggestion is to overall structure the documentation more like reacts. Yew does this to an extent already, but it could be better e.g.

  • The callback documentation on paper makes sense, but when I tried to use it ... it didn't really make sense how to use it. Coming from react I expected to just be able to directly pass closures or functions around, but this wasn't the case. There was a bit of friction and I had to actually go dig through the code (and ask for help in discord) for what I was trying to do (pass a function as a prop to one of my components) - in this case I think just doing a more in depth example with more code could really help here.

TLDR; more code and detail to describe and communicate the concepts

Also, I'm willing to help with more documentation efforts especially with the things I've had a bit of trouble with like passing a function as a prop

Also, as a general question is it just better to put up PRs here rather than just propose things?

@WorldSEnder
Copy link
Member

Also, as a general question is it just better to put up PRs here rather than just propose things?

If you have a ready proposition that improves things, a PR is welcome. If it's unclear what to change and you want to figure things out first, the have a few tags you can apply to your issue, among them documentation, question and ergonomics.

Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

Overall nicer to read. I think the phrasing is still a bit verbose and vague. A link to the context docs could be helpful.

website/docs/concepts/router.mdx Outdated Show resolved Hide resolved
website/docs/concepts/router.mdx Outdated Show resolved Hide resolved
Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

The fixes need to be added in both files

website/docs/concepts/router.mdx Outdated Show resolved Hide resolved
@sasacocic
Copy link
Contributor Author

sasacocic commented Oct 9, 2022

Also, I'll squash these commits together into one whenever merging

@sasacocic
Copy link
Contributor Author

sasacocic commented Oct 14, 2022

Just wanted to bump this it's been a little while.

Copy link
Contributor

@SpanishPear SpanishPear left a comment

Choose a reason for hiding this comment

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

noticed some small typos

website/docs/concepts/router.mdx Outdated Show resolved Hide resolved
website/versioned_docs/version-0.19.0/concepts/router.mdx Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Oct 16, 2022

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

https://yew-rs--pr2913-sasa-update-router-d-29rueg3o.web.app

(expires Thu, 15 Dec 2022 06:19:25 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

website/docs/concepts/router.mdx Outdated Show resolved Hide resolved
website/docs/concepts/router.mdx Outdated Show resolved Hide resolved
@WorldSEnder
Copy link
Member

WorldSEnder commented Oct 18, 2022

Hey. @sasacocic . Just wanted to let you know that I really appreciate your patience on this :D It might feel like this is getting more scrutiny and change requests than a usual documentation change, which I think is because we're nearing release and want to get it exactly right. Thanks for your perseverance and understanding in keeping up with all the different opinions.

@sasacocic
Copy link
Contributor Author

@WorldSEnder no worries :)

@ranile
Copy link
Member

ranile commented Nov 29, 2022

Sorry, I forgot this PR was waiting on me. @sasacocic can you please update the PR so it can be merged. If you don't want to, that's fine. I can do that for you

@sasacocic
Copy link
Contributor Author

@hamza1311 I've updated it. Along with the documentation for 0.20.0. I can revert the change in 0.20.0. if that's necessary. Sorry, for seeing this so late. Got caught up with some stuff.

Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

Thanks for also updating the old docs, there are still quite a lot users around, so it's worth the effort. Only comment from me is fixing some missing quotes for code markup.

website/docs/concepts/router.mdx Outdated Show resolved Hide resolved
@WorldSEnder WorldSEnder enabled auto-merge (squash) December 9, 2022 04:19
@WorldSEnder WorldSEnder merged commit ad4c3fc into yewstack:master Dec 10, 2022
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.

Router breaks if it's in its own module
5 participants