dashboard.jsx: Add environment REPOSITORIES #81
Conversation
src/components/dashboard.jsx
Outdated
| @@ -12,12 +12,24 @@ import CurrentUserStore from '../user-store'; | |||
| import AsyncButton from './async-button'; | |||
| import Time from './time'; | |||
|
|
|||
| const SAMPLE_REPOS = [ | |||
| let SAMPLE_REPOS = [ | |||
There was a problem hiding this comment.
You don't need to change this to let since you still could do push. Also, uppercase words in a variable name must be use const.
src/components/dashboard.jsx
Outdated
| @@ -304,7 +316,6 @@ class ExamplesPanel extends Component { | |||
| {' Example Boards of GitHub Repositories'} | |||
| </span> | |||
| ); | |||
|
|
|||
There was a problem hiding this comment.
Give a new line space for readability.
src/components/dashboard.jsx
Outdated
| {repoOwner: 'huboard', repoName: 'huboard'}, | ||
| {repoOwner: 'openstax', repoNames: ['tutor-js', 'tutor-server'], comment: ' (multiple repositories)'}, | ||
| {repoOwner: 'jquery', repoName: 'jquery'} | ||
| ]; | ||
|
|
||
| try { | ||
| const repos = REPOSITORIES; | ||
| if (repos) { |
There was a problem hiding this comment.
Use double negation characters !!repos to convert it to a boolean. This is much safer.
There was a problem hiding this comment.
Hmm, It makes code harder to read and understand, and it is not used anywhere in the original project.
There was a problem hiding this comment.
Well, if you are fine with it then ok.
src/components/dashboard.jsx
Outdated
| }); | ||
| }; | ||
| } catch (error) { | ||
| console.log('warning:', error); |
There was a problem hiding this comment.
Give a single whitespace after warning:.
There was a problem hiding this comment.
Isn't that done by console.log automatically?
There was a problem hiding this comment.
Ah, you were right. I just see it passed on the second parameter.
There was a problem hiding this comment.
console.warn to get colored output.
|
@jayvdb ready to be merged. Who the one that responsible for this repo, someone with write access that good in JavaScript? |
|
Sorry @wisn, you do not have the necessary permission levels to perform the action. |
src/components/dashboard.jsx
Outdated
| SAMPLE_REPOS.push({ | ||
| repoOwner, repoNames | ||
| }); | ||
| }; |
src/components/dashboard.jsx
Outdated
| @@ -18,6 +18,18 @@ const SAMPLE_REPOS = [ | |||
| {repoOwner: 'jquery', repoName: 'jquery'} | |||
| ]; | |||
|
|
|||
| try { | |||
There was a problem hiding this comment.
What causes this code to require a try/catch?
There was a problem hiding this comment.
When not in production mode, REPOSITORIES will be undefined, leading to an exception.
Maybe I should change that logic a bit in webpack.config.js?
There was a problem hiding this comment.
You have all your code wrapped in an if statement, though, so none of the code will be run if REPOSITORIES is undefined, right?
There was a problem hiding this comment.
Check for the type instead, typeof REPOSITORIES !== 'undefined'
src/components/dashboard.jsx
Outdated
| const repoOwner = repos.split(':')[0]; | ||
| const repoNames = repos.substring(repos.indexOf(':') + 1).split('|'); | ||
| SAMPLE_REPOS.push({ | ||
| repoOwner, repoNames |
There was a problem hiding this comment.
Can you also add comment: ' (from prefetch config)'
or comment: ' (repositories config)' hmm..
There was a problem hiding this comment.
err, i mean comment property :p
see the original SAMPLE_REPOS
li-boxuan
force-pushed
the
fix_routing
branch
2 times, most recently
from
3344c76
to
bafa12b
Compare
|
ack 892b87f |
|
@gitmate-bot ff |
|
Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently |
|
Automated fastforward with GitMate.io was successful! 🎉 |
Closes #3