Skip to content

Resubmitting the milestone for Project 2#40

Open
strisen wants to merge 13 commits into
wdi-sg:masterfrom
strisen:master
Open

Resubmitting the milestone for Project 2#40
strisen wants to merge 13 commits into
wdi-sg:masterfrom
strisen:master

Conversation

@strisen

@strisen strisen commented Jan 1, 2018

Copy link
Copy Markdown

Deliverable Submission

Please describe your comfort and completeness levels before submitting.

Comfort Level (1-5): 4

Completeness Level (1-5): 3

What did you think of this deliverable?: Working with APIs was more complicated than I thought. Main reason for this was the lack of details in the documentations provided, and required a lot of research + testing.

Comment thread public/javascript/ui.js Outdated
@@ -0,0 +1,46 @@
(function (window, document) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm why are we splitting the javascript file here? any major reasons?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and why we are using iife method?

Comment thread views/home.handlebars Outdated
<div class="l-content">
<div class="pricing-tables pure-g">
<div class="pure-u-1 pure-u-md-1-2">
<div class="pricing-table pricing-table-biz pricing-table-selected">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the class name is pricing-table?

Comment thread app.js
app.set('view engine', 'handlebars')

// Routes
app.use('/', routes)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of your operations happened in sockets, where are the CRUD database operation?

@primaulia

Copy link
Copy Markdown
Contributor

Nice to have:

  • A better color choice for flash message (red for logout / warning, green for success, etc)
  • I think the / should be your train tracker delays page from the get go. It's okay to hide the status update if you have not logged in, but I suggest to combine both / and /home together

Comment thread app.js
const routes = require('./routes/routes')
const dbConfig = require('./config/dbConfig')
const Twit = require('twit')
const tweet = require('./helpers/twitter')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you should group all of your helper files under ./helpers/ directory together

Comment thread app.js
tweet.get('search/tweets', {q: query, count: 50, tweet_mode:'extended', result_type:'reverse'}, function(err, data, res){
if(err)(console.log(err))
for(let i=0; i<data.statuses.length; i++){
if(data.statuses[i].retweeted_status){

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove this if condition if it's not needed

Comment thread app.js

tweet.get('search/tweets', {q: query, count: 50, tweet_mode:'extended', result_type:'reverse'}, function(err, data, res){
if(err)(console.log(err))
for(let i=0; i<data.statuses.length; i++){

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try to use forEach instead perhaps?

Comment thread routes/routes.js
// Tracker Page
router.get('/home', isLoggedIn, HomeController.home)

router.post('/create', HomeController.create)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should check all post, put, and delete routes too if the current user is authenticated or not too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants