NYPL / staff-picks

1 stars 3 forks source link

Www 17 Staff Picks Routing #169

Closed holingpoon closed 6 years ago

holingpoon commented 6 years ago

Will work with @ktp242 on resolving conflicts in monthData.js, and figure out branch merging scheme. Thanks!

EdwinGuzman commented 6 years ago

Fix the merge conflict and try to fix the tests too. The broken tests are not related to your work, but they should still work given your update.

EdwinGuzman commented 6 years ago

@ktp242 eslint complains about the trailing comma because it's a list. It doesn't matter if it's an array and there's a list there, or if they're function params, they're still lists.

holingpoon commented 6 years ago

@EdwinGuzman So should there be commas after the last item? Should they stay off for best practices?

EdwinGuzman commented 6 years ago

If we're following eslint, yes, the commas should be there. Also, the local import statements should not have an extension at the end (jsx, js);

holingpoon commented 6 years ago

Okay, @ktp242 @EdwinGuzman I kept the trailing commas. I also removed .jsx extensions from the import statements.

ktp242 commented 6 years ago

I remember once I ran into a broken function because a trailing , attached to the arguments. Just want to make sure it doesn't break anything.

holingpoon commented 6 years ago

@ktp242 That is also my concern, localhost seems to be tolerant of trailing commas right now. Maybe deploying to development will give us better clues?

EdwinGuzman commented 6 years ago

When you deploy, the app will be serving the translated and compiled JS which will remove the commas I think (when babel does the transpilation). Locally, the app is serving the jsx files so if it works locally then it should work when deployed, but it's go to check either way.

EdwinGuzman commented 6 years ago

Works well.