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(guide) Replace invalid prism code block names #35961

Conversation

@RandellDawson
Copy link
Member

commented May 3, 2019

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.
  • None of my changes are plagiarized from another source without proper attribution.
  • All the files I changed are in the same world language (for example: only English changes, or only Chinese changes, etc.)
  • My changes do not use shortened URLs or affiliate links.

Related to #35960

This PR aims to resolve 95% of the issues related to issue #35960. I will create a separate PR for the other 5% based on feedback given to my comment on the issue.

Since my changes only effect code blocks, I put everything on a single PR (note I did not check the box above regarding multiple languages on the same PR). If these need to be broken down by language, I can do that if absolutely necessary.

I have kept all the commits (one for each type of replacement made) in case I need to revert any. At the very end, I can squash them if necessary.

@RandellDawson

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

OK, I no longer see the prism warnings after my latest commits.

@ojeytonwilliams ojeytonwilliams self-requested a review May 6, 2019

@ojeytonwilliams
Copy link
Contributor

left a comment

Nice work! There were a few ```sh and ```C++ that slipped through, but for the most part it looks great. I didn't check the formatting of every file, but I did look at a semi-random sample and they all seemed fine. After our discussion, I checked the XAML -> xml change and that looks fine to me.

One thing I wasn't sure how to do was build the guide in any language other than English. Any ideas?

Just restate, because the comment order isn't what I thought it would be: there were some jsxs used in the redux section, but that's just pure JS.

Show resolved Hide resolved guide/portuguese/cplusplus/terms-to-know-for-beginners/index.md Outdated
Show resolved Hide resolved guide/portuguese/csharp/hello-world/index.md Outdated
Show resolved Hide resolved guide/chinese/csharp/hello-world/index.md Outdated
Show resolved Hide resolved ...rtifications/front-end-libraries/redux/dispatch-an-action-event/index.md Outdated
Show resolved Hide resolved ...ations/front-end-libraries/redux/get-state-from-the-redux-store/index.md Outdated
Show resolved Hide resolved guide/spanish/cplusplus/terms-to-know-for-beginners/index.md Outdated
Show resolved Hide resolved guide/spanish/csharp/hello-world/index.md Outdated
Show resolved Hide resolved guide/spanish/rust/installing-rust/index.md Outdated
Show resolved Hide resolved guide/spanish/redux/tutorial/index.md Outdated
Show resolved Hide resolved guide/spanish/rust/installing-rust/index.md Outdated

@freeCodeCamp freeCodeCamp deleted a comment from ojeytonwilliams May 8, 2019

@RandellDawson

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@ojeytonwilliams Thanks for the review. Not sure how those extra lines got in there, but I must have messed up my regex replace in VS Code somehow. I have committed all of your suggestions.

One thing I wasn't sure how to do was build the guide in any language other than English. Any ideas?

I don't think the logic has been completely implemented yet but @raisedadead could answer better.

@raisedadead

This comment has been minimized.

Copy link
Member

commented May 8, 2019

You can locally build the guide or learn for available languages by changing the LOCALE=english in your .env files.

@RandellDawson

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@raisedadead followup question: When we use npm run develop, then guide is really just the mock-guide files. Is there a way to use npm run develop so that the complete guide is built out for a particular language using the actual guide folder?

@ojeytonwilliams

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@RandellDawson Revised dirty hack - you can replace mock-guide with guide everywhere it appears in the client (client/plugins/fcc-create-nav-data/create-navigation-node.js, client/gatsby-config.js and client/utils/getGithubPath.js) and it should work fine.

@raisedadead Yeah, I tried that, but there are errors in the challenge files, so gatsby develop never finishes. I guess the solution is to just not build the challenges. I'll see if I can do that without breaking things!

@raisedadead

This comment has been minimized.

Copy link
Member

commented May 8, 2019

With languages other than English you have to build the production bundle. Spoiler you need a really good machine that will survive the build.

Then you can spawn the api app and the client app separately.

You want gatsby serve in client and dig into the server package json for the script to spin up the api.

@moT01

This comment has been minimized.

Copy link
Member

commented May 9, 2019

You can copy all the stuff from the guide folder and paste in the mock-guide folder - that's how I usually test guide changes.

@RandellDawson

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Moderators: It appears we are going to have to wait a while to merge this one, because there are several PRs (across multiple languages) before it which would result in merge conflicts if merged first. I will start reviewing the English guide articles, but we will need members of the non-English language teams to help review the other non-English PRs.

@raisedadead

This comment has been minimized.

@moT01

This comment has been minimized.

Copy link
Member

commented May 10, 2019

looking at the contributors tool @RandellDawson - it looks like maybe 50 files have other PR's touching these files - you could remove those files from this PR and there would be no potential for conflicts - and we could maybe merge this faster

@RandellDawson

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

@moT01 I suppose I can revert the changes to the 42 files on those specific earlier PRs and then go into each and add the prism support language to the triple backticks if needed. That way, they will get the update, but not end up with a merge conflict. What do you think? I think I can write a short bash script to checkout the files, but I think I would need to squash all the existing commits into a single commit first locally before doing that.

RandellDawson added some commits May 3, 2019

fix: replace sh with shell
fix replace terminal with shell

fix replace node with js

fix replace output with shell

fix replace cs with csharp

fix replace c++ with cpp

fix replace c# with csharp

fix replace javasctipt with js

fix replace syntax  with js

fix replace unix with shell

fix replace linux with shell

fix replace java 8 with java

fix replace swift4 with swift

fix replace react.js with jsx

fix replace javascriot with js

fix replace javacsript with js

fix replace c++ -  with cpp

fix: corrected various typos

fix: replace Algorithm with nothing

fix: replace xaml with xml

fix: replace solidity with nothing

fix: replace c++ with cpp

fix: replace txt with shell

fix: replace code with json and css

fix: replace console with shell

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: changed jsx to js

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: changed jsx to js

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: changed jsx to js

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

fix: removed line not needed

Co-Authored-By: RandellDawson <5313213+RandellDawson@users.noreply.github.com>

@RandellDawson RandellDawson force-pushed the RandellDawson:fix/replace-invalid-prism-code-block-names branch from 5a45371 to 806f61a May 10, 2019

@RandellDawson

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

I have successfully reverted the changes to the 40+ files on earlier PRs which would have caused merge conflicts if this PR would have been merged. Now, I will review the 68 PRs which had these 42 files and add an extra commit to fix the invalid prism code block names if needed.

@RandellDawson

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

OK. I just went through the 68 PRs which contained the files 42 files I reverted earlier. I made commits on those PRs so if they are merged, they will already have the correct code block names.

@moT01
Copy link
Member

left a comment

I took a look through these - nice job @RandellDawson! I found these few things that I think got missed. Other than those I think the rest looks good.

I did not test this locally - I simply looked through the diff

Show resolved Hide resolved guide/chinese/redux/tutorial/index.md Outdated
Show resolved Hide resolved guide/chinese/rust/installing-rust/index.md Outdated
Show resolved Hide resolved guide/portuguese/redux/tutorial/index.md Outdated
Show resolved Hide resolved guide/portuguese/rust/installing-rust/index.md Outdated
Show resolved Hide resolved guide/portuguese/rust/installing-rust/index.md Outdated

RandellDawson and others added some commits May 12, 2019

fix: removed line not needed
Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>
fix: removed line not needed
Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>
fix: removed line not needed
Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>
fix: removed line not needed
Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>
fix: removed line not needed
Co-Authored-By: Tom <20648924+moT01@users.noreply.github.com>
@RandellDawson

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

@moT01 OK, I committed the changes which I had missed. I am thinking we should probably have one more moderator take a look at this PR before it gets merged. I am going to do one more look myself to make sure I can not find anything else either.

@moT01

moT01 approved these changes May 12, 2019

@Manish-Giri
Copy link
Collaborator

left a comment

LGTM 💯

@ojeytonwilliams

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@RandellDawson you mentioned taking another look - are you happy with the PR?

@RandellDawson

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

@ojeytonwilliams Yes I am happy with it now. Feel free to merge it. Once this one is merged, I will be creating another PR for a related issue which deals with a ton of files which have a space between the triple backticks and the language specifier (like below).

``` js
// code goes here
```

@ojeytonwilliams ojeytonwilliams merged commit 0a1eeea into freeCodeCamp:master May 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ojeytonwilliams

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Thanks @RandellDawson for the hard work and @moT01 and @Manish-Giri for reviewing this monster!

@RandellDawson RandellDawson deleted the RandellDawson:fix/replace-invalid-prism-code-block-names branch May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.