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

fix: made necessary changes to docusaurus #123

Closed
wants to merge 3 commits into from

Conversation

ashwinijha6
Copy link

@ashwinijha6 ashwinijha6 commented Mar 12, 2021

Description

Made necessary changes to docusaurus :

  • Add the command npm run serve wherever required in README.md in docs/
  • Removed .features & .featureImage from here
  • Changed the link for Backend docs in the docusaurus.config.js
  • Removed exclamation from Home.md

Fixes #115

Type of Change:

  • Code
  • Quality Assurance
  • Documentation

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update (software upgrade on readme file)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged
  • I have written Kotlin Docs whenever is applicable

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

@ashwinijha6
Copy link
Author

@keshakaneria & @codesankalp could u'll pls review this PR

keshakaneria
keshakaneria previously approved these changes Mar 13, 2021
Copy link
Member

@keshakaneria keshakaneria left a comment

Choose a reason for hiding this comment

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

Looks good 🎉

Copy link
Member

@codesankalp codesankalp 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 adding a line for yarn also, will be a good option.

docs/README.md Outdated
@@ -24,6 +24,8 @@ yarn build

This command generates static content into the `build` directory and can be served using any static contents hosting service.

**Note**: You can use `npm run serve` to test your build locally.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**Note**: You can use `npm run serve` to test your build locally.
**Note**: You can use `yarn serve` to test your build locally.

Copy link
Author

@ashwinijha6 ashwinijha6 Mar 13, 2021

Choose a reason for hiding this comment

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

@codesankalp could you pls explain why I should replace npm run serve with yarn serve ? I don't see much difference in it, but I guess yarn is much faster than npm.

Copy link
Member

Choose a reason for hiding this comment

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

@ashwinijha6 You have added the same note line twice. As the first few lines of the README include yarn start, yarn build etc.. then according to the flow there should be yarn serve for testing the build locally.
I think it is a valid reason.

Copy link
Author

Choose a reason for hiding this comment

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

Yes that was on purpose @codesankalp. I can do one thing that is change the command after yarn build to yarn serve & let the second command after npm run build be as it is OR remove the second command. What do you think?

@codesankalp codesankalp added the Status: Changes Requested Changes are required to be done by the PR author. label Mar 13, 2021
@ashwinijha6
Copy link
Author

@keshakaneria & @codesankalp just I have made the changes required

@codesankalp codesankalp added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Mar 15, 2021
Copy link
Member

@Amulya-coder Amulya-coder left a comment

Choose a reason for hiding this comment

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

@ashwinijha6 QA checks are failing please run the linter and formatter as mentioned in the readme.

@ashwinijha6
Copy link
Author

Hey @Amulya-coder I did this error while running npm run lint:fix Should I perform this before running it npm install --save-dev eslint-config-prettier
eslint

@codesankalp
Copy link
Member

@ashwinijha6 Delete your node_modules and package-lock.json.
and try npm install.

@ashwinijha6
Copy link
Author

@ashwinijha6 Delete your node_modules and package-lock.json.
and try npm install.

Ok I'll try that @codesankalp

@isabelcosta
Copy link
Member

Will close this due to lack of update. cc @codesankalp
@ashwinijha6 feel free to claim this again if you're interested!

@isabelcosta isabelcosta closed this Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: add required changes to docusaurus present in /docs
5 participants