Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resubmitting the milestone for Project 2 #40

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Resubmitting the milestone for Project 2 #40

wants to merge 13 commits into from

Conversation

strisen
Copy link

@strisen strisen commented Jan 1, 2018

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.

@@ -0,0 +1,46 @@
(function (window, document) {
Copy link
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
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?

<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
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?

app.set('view engine', 'handlebars')

// Routes
app.use('/', routes)
Copy link
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
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

const routes = require('./routes/routes')
const dbConfig = require('./config/dbConfig')
const Twit = require('twit')
const tweet = require('./helpers/twitter')
Copy link
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

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
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


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
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?

// Tracker Page
router.get('/home', isLoggedIn, HomeController.home)

router.post('/create', HomeController.create)
Copy link
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.

None yet

2 participants