-
Notifications
You must be signed in to change notification settings - Fork 131
docs: Add RapidAPI guide #2015
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
docs: Add RapidAPI guide #2015
Conversation
Preview for this PR was built for commit |
Preview for this PR was built for commit |
Thanks for new content! I see you're adding this to the Academy, but in the future we'd like the main section of Academy to contain only courses, i.e. something larger that consists of a series of lessons, where the reader works on building something (ideally also learning something by accident 😄). This looks more like a how-to guide, which, in my mind, fits better to the Tutorials section. That section needs more love and the structure is TBD, but I'd still put this content there. What do you think? btw, I see a change to the |
Hi @honzajavorek. Thanks for the feedback. I moved it to tutorials. Let me know what do you think, thank you! |
Preview for this PR was built for commit |
Preview for this PR was built for commit |
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.
Awesome, thank you! I performed a thorough review, left a lot comments, but please don't feel intimidated. They're mostly nitpicks. In fact I found no really serious issues! My summary:
- I'm unsure if we want to write tutorials as "you" or "we". That's something we should discuss with @TC-MO. I have no strong opinions, it's more a matter of (future) consistency.
- Admonitions are almost there, I'd add one more and improve some of the existing ones.
- At places, English could be improved.
- Some code examples have wrong highlighting.
- I tried to suggest improvements to the
apify push
part, so that we carry people over the login trap more carefully. - I suggested consistent use of bold for UI elements.
- I'm unsure about the .mdx / .md extensions
sources/academy/tutorials/apify_actors/adding_rapidapi_project.mdx
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.mdx
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.mdx
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.mdx
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.mdx
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.mdx
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.mdx
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.mdx
Outdated
Show resolved
Hide resolved
Hi @honzajavorek, thanks! I'll check it.
It was checked by our content team, so I assumed it would be OK, but if you think it needs improvement, let's do it. :) |
Aaah, well, I'm no native speaker nor English expert, I'll be honest about that 😄 I wrote some suggestions according to how I feel it, but it's up to you if you think they're something to address or if you discard them. The whole text is a good job overall, it's comprehensible as is, so take those English grammar/style comments just as nitpicks and feel free to jump over them. |
….mdx Co-authored-by: Honza Javorek <mail@honzajavorek.cz>
….mdx Co-authored-by: Honza Javorek <mail@honzajavorek.cz>
….mdx Co-authored-by: Honza Javorek <mail@honzajavorek.cz>
Co-authored-by: Honza Javorek <mail@honzajavorek.cz>
Co-authored-by: Honza Javorek <mail@honzajavorek.cz>
Preview for this PR was built for commit |
Preview for this PR was built for commit |
sources/academy/tutorials/apify_actors/adding_rapidapi_project.md
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.mdx
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.mdx
Outdated
Show resolved
Hide resolved
Preview for this PR was built for commit |
@TC-MO @honzajavorek thanks for the review, I think that everything is fixed. |
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 noticed few more things:
- reworded description and intro sentence - this is just a suggestion
- consistency in naming - would be nice to have
- removal of the - this is per Apify style guide, IIRC we do not use
the
before Apify Console
sources/academy/tutorials/apify_actors/adding_rapidapi_project.md
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.md
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.md
Outdated
Show resolved
Hide resolved
sources/academy/tutorials/apify_actors/adding_rapidapi_project.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Michał Olender <92638966+TC-MO@users.noreply.github.com>
Preview for this PR was built for commit |
Preview for this PR was built for commit |
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.
If Michał approves, it's good to go, i.e. don't wait for a re-review from me 🚀
I didn't manage to remove myself as a reviewer in the GitHub mobile app. If my previous review blocks merging this, let me know and I'll rubber stamp this as approved, as a sacrifice to the GitHub UI. |
I think your approve is required @honzajavorek since you've requested changes previously |
No description provided.