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

Document html_nested! macro and add regression doc-tests for #1527 #1530

Merged
merged 5 commits into from
Aug 29, 2020

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Aug 27, 2020

Description

Documentation for html_nested!, typical usage and regression tests for #1527 in documentation

Checklist:

  • I have run ./ci/run_stable_checks.sh
  • I have reviewed my own code
  • I have added tests

@siku2 siku2 linked an issue Aug 27, 2020 that may be closed by this pull request
1 task
@siku2
Copy link
Member

siku2 commented Aug 27, 2020

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 27, 2020

Command update: success

Branch has been successfully updated

Copy link
Member

@siku2 siku2 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this! I left a few comments for the code snippets but there are also a few language issues that should be fixed.
Perhaps @teymour-aldridge is willing to take a look? Otherwise I'll do it tomorrow.

yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@teymour-aldridge teymour-aldridge left a comment

Choose a reason for hiding this comment

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

I think that some of this should move from the doc comment into the accompanying documentation.
This is just an initial pass picking up some spelling/grammar errors.
The suggestions are just suggestions, and are more to point out where I think there are mistakes than my pushing for my suggested phrasing/wording to be adopted.

yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
@siku2
Copy link
Member

siku2 commented Aug 28, 2020

@teymour-aldridge, I agree, at the very least html_nested! should be mentioned in the documentation. I think that should be done through a follow-up PR though.

@teymour-aldridge
Copy link
Contributor

I'm thinking that there's too much here for a doc comment and some of this should be moved into the docs.

@siku2
Copy link
Member

siku2 commented Aug 28, 2020

Yeah, especially compared to the API docs of the html! macro. It makes more sense to explain the intrinsic details in the documentation. I'm saying we should leave it as is for this PR so we at least have something for now and then move it to the documentation in another PR so as not to put more work on @Mingun.

@teymour-aldridge
Copy link
Contributor

Sounds like a plan!

@Mingun
Copy link
Contributor Author

Mingun commented Aug 28, 2020

I've fix language, changed From to Into and add small note to html! about existing html_nested! and manual.

Yeah, especially compared to the API docs of the html! macro. It makes more sense to explain the intrinsic details in the documentation.

Actually, I'm prefer to have some very basics concepts in the rustdoc too. At least basic example of using html!, and example for dynamic tags (the last thing even isn't documented in manual). Manual mostly for deep understanding concepts, rustdoc -- for dirty copying examples of code :)

@Mingun Mingun requested a review from siku2 August 28, 2020 18:11
Copy link
Member

@siku2 siku2 left a comment

Choose a reason for hiding this comment

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

Just a few issues left.

yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
yew/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Simon <simon@siku2.io>
@Mingun
Copy link
Contributor Author

Mingun commented Aug 29, 2020

Failed documentation tests -- GitHub issue.

@Mingun Mingun requested a review from siku2 August 29, 2020 07:57
@siku2 siku2 merged commit a04c78b into yewstack:master Aug 29, 2020
@siku2
Copy link
Member

siku2 commented Aug 29, 2020

Thanks, @Mingun!

@siku2 siku2 added this to the v0.18 milestone Aug 29, 2020
@Mingun Mingun deleted the document-html_nested branch August 29, 2020 16:35
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.

Add documentation for html_nested! macro
3 participants