fixed bold name error in some templates #20

avirlrma · 2017-06-05T05:08:41Z

#19 Fixed the issue!
Change was required in templates 2,4,6.I also removed some extra code.
So earlier in template 2,4,6, "Richard P. Hendricks" looked like "Richard P. Hendricks" now it should look like "Richard P. Hendricks"
Could you @saadq test and merge this?
BTW You were really helpful, made my work so much easier considering the fact that this was my first good PR.
Thanks!

saadq

Nice work! Thanks for your contribution.

Before I can accept this pull request, there are a few things to take note of:

You have some duplicate files committed. It seems for each template, you have a duplicate index.js file. Just be sure to remove that from the commit.
Your indentation/whitespace has some issues. Just make sure to run npm run lint to figure out what you should change. npm run fix will automatically fix some of the issues for you. npm run format will automatically keep the formatting of the code consistent.

When you are all done with these changes, make sure to run npm test to make sure everything is working right. Just be sure to stop the server if it's running in order to allow the test to work, otherwise there might be issues when it tries to start its own server.

saadq · 2017-06-05T05:29:15Z

app/server/src/generator/templates/template2/index.js

-      nameEnd = names[names.length - 1]
+    }  else {
+	  nameStart = names[0]
+      nameEnd = names.slice(1,names.length).join(' ')
    }


Indentation is a bit messed up here, and I believe it might be because you're using a tab somewhere. The project is using 2-space indent.

saadq · 2017-06-05T05:29:35Z

app/server/src/generator/templates/template2/index.js~

+  `
+}
+
+module.exports = template2


This file shouldn't exist.

saadq · 2017-06-05T05:29:58Z

app/server/src/generator/templates/template4/index.js

-      nameStart = names.slice(0, names.length - 1).join(' ')
-      nameEnd = names[names.length - 1]
+      nameStart = names[0]
+      nameEnd = names.slice(1,names.length).join(' ')
    }
  }



saadq · 2017-06-05T05:30:11Z

app/server/src/generator/templates/template4/index.js~

+  `
+}
+
+module.exports = template4


This file also shouldn't be committed.

saadq · 2017-06-05T05:30:30Z

app/server/src/generator/templates/template6/index.js

-      nameStart = names.slice(0, names.length - 1).join(' ')
-      nameEnd = names[names.length - 1]
+     nameStart = names[0]
+     nameEnd = names.slice(1,names.length).join(' ')
    }
  }



Same as before, just make sure the indentation/whitespace is consistent with the project.

saadq · 2017-06-05T05:30:39Z

app/server/src/generator/templates/template6/index.js~

+  `
+}
+
+module.exports = template6


Just remove this file as well!

avirlrma · 2017-06-05T10:02:49Z

I guess I did the neccesary things. Sorry for the indentation thing, I am in habit of tabs.I'm not sure about the npm thing.So please review the changes and merge it if it's all good.

saadq · 2017-06-05T10:35:47Z

Are you unfamiliar with running npm scripts? Do you have node and npm installed? You can run:

$ npm run lint

in the command line, and it will tell you about any formatting/linting errors. I had listed a few of them although there are a couple more. Rather than listing them all in a review, could you just run npm run lint in the command line so it will list all of the small minor formatting issues you could fix. Let me know if you need further assistance.

Edit: I've actually posted a second review below, so it isn't necessary for you to run the npm scripts yourself although it would be good practice in case you ever want to contribute to a node project again.

saadq · 2017-06-05T10:51:18Z

app/server/src/generator/templates/template2/index.js

-      nameStart = names.slice(0, names.length - 1).join(' ')
-      nameEnd = names[names.length - 1]
+      nameStart = names[0]
+      nameEnd = names.slice(1,names.length).join(' ')


nit: add a space after the comma. 1, names.length

saadq · 2017-06-05T10:51:30Z

app/server/src/generator/templates/template4/index.js

-      nameStart = names.slice(0, names.length - 1).join(' ')
-      nameEnd = names[names.length - 1]
+      nameStart = names[0]
+      nameEnd = names.slice(1,names.length).join(' ')


same as above.

saadq · 2017-06-05T10:51:35Z

app/server/src/generator/templates/template6/index.js

-      nameStart = names.slice(0, names.length - 1).join(' ')
-      nameEnd = names[names.length - 1]
+      nameStart = names[0]
+      nameEnd = names.slice(1,names.length).join(' ')


saadq · 2017-06-05T10:53:01Z

For simplicity, I've also posted the linting issues here in a new review. The project follows a pretty strict style guide, so little things like spacing will make the tests fail. It's usually pretty easy to fix if you use npm run fix in the command line to automatically fix linting issues or npm run format to automatically make style consistent. But if you don't have npm available, you can just fix those issues I just mentioned in the above review.

avirlrma · 2017-06-05T12:26:40Z

I guess it's all done?

avirlrma · 2017-06-05T12:56:46Z

@saadq Thanks man!
Learned a lot!
Sorry for trouble caused, I'm just a noob.
Would you mind If I work on more Issues?

saadq · 2017-06-05T13:12:06Z

Hey @aviral1701, thanks again for the contribution! It's much appreciated. I would be glad to have some help on the issues, although I would definitely first recommend setting up your environment if you haven't already. By that I mean having node and npm installed so that you can run the app locally and have access to all the npm scripts like linting, testing, formatting, etc so that code reviews are a lot easier and we can focus on the code itself instead of trivial things like indentation and formatting.

I've already start working on #6, #7, and #8, and have some plans for #14, but if you have any interest in the other ones feel free to tackle them.

If you are interested in any of them just comment in the issue and I can provide whatever info (if any) I already have about the issue! :)

saadq · 2018-02-05T13:12:52Z

@aviral1701 I just added you as a contributor to the README, let me know if you'd rather not be on there. Thanks your contribution :)

avirlrma · 2018-02-05T13:38:08Z

Hey @saadq. Thanks for adding me in the contributors. Your project was my first open source contribution,so thanks for that as well. See ya ;)

fixed bold name error in some templates

3b738d5

saadq requested changes Jun 5, 2017

View reviewed changes

avirlrma added 5 commits June 5, 2017 15:11

fixed indentation

c508a8d

Delete index.js~

944e4e7

Delete index.js~

69375c2

Delete index.js~

c1ea83b

removed some weird stuff

fa99e0b

saadq requested changes Jun 5, 2017

View reviewed changes

avirlrma added 2 commits June 5, 2017 17:49

some more stupid things

9fe2e04

Merge branch 'master' of https://github.com/aviral1701/latexresu.me

75dee51

saadq approved these changes Jun 5, 2017

View reviewed changes

saadq merged commit dd45110 into saadq:master Jun 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed bold name error in some templates #20

fixed bold name error in some templates #20

avirlrma commented Jun 5, 2017

saadq left a comment

saadq Jun 5, 2017

saadq Jun 5, 2017

saadq Jun 5, 2017

saadq Jun 5, 2017

saadq Jun 5, 2017

saadq Jun 5, 2017

avirlrma commented Jun 5, 2017

saadq commented Jun 5, 2017 •

edited

Loading

saadq Jun 5, 2017

saadq Jun 5, 2017

saadq Jun 5, 2017

saadq commented Jun 5, 2017 •

edited

Loading

avirlrma commented Jun 5, 2017

avirlrma commented Jun 5, 2017

saadq commented Jun 5, 2017

saadq commented Feb 5, 2018

avirlrma commented Feb 5, 2018

fixed bold name error in some templates #20

fixed bold name error in some templates #20

Conversation

avirlrma commented Jun 5, 2017

saadq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avirlrma commented Jun 5, 2017

saadq commented Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saadq commented Jun 5, 2017 • edited Loading

avirlrma commented Jun 5, 2017

avirlrma commented Jun 5, 2017

saadq commented Jun 5, 2017

saadq commented Feb 5, 2018

avirlrma commented Feb 5, 2018

saadq commented Jun 5, 2017 •

edited

Loading

saadq commented Jun 5, 2017 •

edited

Loading