dijs / wiki

Wikipedia Interface for Node.js
MIT License
315 stars 61 forks source link

Page Coordinates Update 2.0 #38

Closed TedYav closed 7 years ago

TedYav commented 7 years ago

Summary of modifications: (Edit: squashed commits into one)

Got a chance to work on this this weekend, cheers! Lemme know if you want me to clean it up or make any changes. This npm module helped me out a ton on a project I was doing, so figured I could patch up one of the edgecases on it đź‘Ť

coveralls commented 7 years ago

Coverage Status

Coverage increased (+2.8%) to 92.946% when pulling c90a7d41723f267a7897a7c24a3a31c6076cd4f4 on TedYav:coord-update into c81e2fd512d4a34874fedabae11a4ced2b0d6656 on dijs:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+2.8%) to 92.946% when pulling 85dd92f79ddbfeda0ad89e66fc1c87f661e2fd5a on TedYav:coord-update into c81e2fd512d4a34874fedabae11a4ced2b0d6656 on dijs:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+2.8%) to 92.946% when pulling 2cc22df0fc3ce4f73831fc43e74d5a5156a0a7e5 on TedYav:coord-update into c81e2fd512d4a34874fedabae11a4ced2b0d6656 on dijs:master.

dijs commented 7 years ago

Dude! Thank you for all this work. I will try to review it a bit more closely today and get it merged soon.

dijs commented 7 years ago

Also, did you run eslint? It looks like there are a few little code style tweaks that need to be made.

TedYav commented 7 years ago

Will make changes—cheers and thanks for feedback—you caught a few things I forgot to clean.

I did install ESlint plugin into VSCode and ran ESlint on it—didn't get any issues. Just tested it by adding extra spaces and it gave me an error so perhaps .estlintrc doesn't contain all desired rules? If you point out some examples of code style issues I'll fix them.

TedYav commented 7 years ago

Q: would you like me to make changes and then rebase so that there's a single commit or create a new commit? Thanks!

dijs commented 7 years ago

A new commit is great because I can easily see your changes. I normally use rebasing only locally when I want to work on multiple tasks locally, then rebase them into one and push up. Then after my colleagues review, I add more commits.

TedYav commented 7 years ago

:+1: thanks for suggestions -- made changes as suggested. Also factored out function to reduce tons of duplicated lines.

EDIT: ran eslint — didn't catch any code style issues but perhaps it's not configured for all of them — lemme know if there's any other you want me to change. Cheers.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+6.4%) to 96.575% when pulling 973db438d2e8fd3ca755a17eb27764c4fb319965 on TedYav:coord-update into c81e2fd512d4a34874fedabae11a4ced2b0d6656 on dijs:master.