-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
add tutorial for content filtering subscription. #2441
add tutorial for content filtering subscription. #2441
Conversation
Signed-off-by: Tomoya.Fujita <[email protected]>
Signed-off-by: Tomoya.Fujita <[email protected]>
d062dcd
to
990b08d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a bunch of small changes inline. I think we'll be good to go once that is in (though I'd also like to get @wjwwood to review before we merge it).
@clalancette appreciate for many suggested fixes, i addressed them as suggested. |
Signed-off-by: Tomoya.Fujita <[email protected]> Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
I've done a few more grammar changes in abee316 . At this point, this looks good to me, so I'm going to approve. @fujitatomoya I'd appreciate one more look from you just to make sure my latest changes are good with you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
I have a lot of minor wording suggestions.
It would also be nice to not copy paste the code blocks, is it possible to link to the actual code instead?
Signed-off-by: Tomoya.Fujita <[email protected]> Co-authored-by: Ivan Santiago Paunovic <[email protected]>
i thought that would be nice too, but not sure how we can make it. i do not see any examples in current ros2_documents either. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
We've always wanted to have a way to reference the content in the git repo, but we never implemented it.
* add tutorial for content filtering subscription. Signed-off-by: Tomoya.Fujita <[email protected]> Co-authored-by: Chris Lalancette <[email protected]> Co-authored-by: Ivan Santiago Paunovic <[email protected]> (cherry picked from commit 7408ded)
* add tutorial for content filtering subscription. Signed-off-by: Tomoya.Fujita <[email protected]> Co-authored-by: Chris Lalancette <[email protected]> Co-authored-by: Ivan Santiago Paunovic <[email protected]> (cherry picked from commit 7408ded)
* add tutorial for content filtering subscription. Signed-off-by: Tomoya.Fujita <[email protected]> Co-authored-by: Chris Lalancette <[email protected]> Co-authored-by: Ivan Santiago Paunovic <[email protected]> (cherry picked from commit 7408ded) Co-authored-by: Tomoya Fujita <[email protected]>
aligned with ros2/demos#557
Signed-off-by: Tomoya.Fujita [email protected]