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.

Sign up for GitHub

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

Jump to bottom

Correcting documentation #23

Merged
merged 14 commits into from
Mar 14, 2022
Merged

Correcting documentation #23

merged 14 commits into from
Mar 14, 2022

Conversation

denis-rossati
Copy link
Member

@denis-rossati denis-rossati commented Jan 27, 2022 •
edited
Loading

Summary

  • Colocar exemplos completos, que possam ser copiados e colados 100% funcionais
  • No readme, ao invés de colocar o APP ID de sandbox, colocar algo como YOUR_APP_ID por algo como "Replace YOUR_APP_ID with your public app ID that you can find at Application > Setup on your account." Acho que vale ressaltar essa parte, talvez usando um callout.
  • Typo na sessão “Plugging in” <CroctProvider/ > para (double check se temos outros casos assim)
  • No exemplo com Next usar o componente Script (next/script) ao invés da tag script

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas There is no hard-to-understand areas
  • I have made corresponding changes to the documentation Still have to change the plug.js as well
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works No need for tests
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Correspondent PR in plug.js

marcospassos
marcospassos requested changes Jan 27, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
Fryuni
Fryuni previously approved these changes Feb 1, 2022
View reviewed changes
@Fryuni Fryuni dismissed their stale review February 1, 2022 20:28

GitHub only showed part of the diff

marcospassos
marcospassos requested changes Feb 1, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
examples/next-ssr-app/pages/_app.tsx Outdated Show resolved Hide resolved
@denis-rossati
Copy link
Member Author

denis-rossati commented Feb 2, 2022 •
edited
Loading

Funny enough, the <Script /> inside the <Head /> in Next wasn't loading the custom.js and, consequently, wasn't loading the croct.fetch. The example was wrong, so I said "I'll put outside the <Head /> and see what happens" and then the script worked!

I never used Next.js so I do not know the details about putting the <Script /> inside the <Head /> tag or not. That made me think "If the script is not inside the head, where is it?" and I take a look in the HTML of the example and the script was right in the middle of the <head> tag... Next is mysterious

marcospassos
marcospassos requested changes Feb 2, 2022
View reviewed changes
README.md Show resolved Hide resolved
@marcospassos marcospassos added documentation Improvements or additions to documentation enhancement New feature or request labels Mar 14, 2022
@marcospassos marcospassos merged commit 9d35734 into croct-tech:master Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcospassos marcospassos marcospassos requested changes

@Fryuni Fryuni Fryuni left review comments

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@denis-rossati @marcospassos @Fryuni