-
Notifications
You must be signed in to change notification settings - Fork 1
Samipublicchat #18
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
base: master
Are you sure you want to change the base?
Samipublicchat #18
Conversation
claireinez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please get rid of the console logs and commented out code!
config.env
Outdated
| @@ -0,0 +1 @@ | |||
| DATABASE_URL= postgres://postgress:12345678@localhost:5432/cryptodb | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file shouldn't be in source control
src/backEnd/SocketManager.js
Outdated
| @@ -0,0 +1,91 @@ | |||
| const { io } = require('./server.js'); | |||
| // console.log('poopoo', io); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
src/backEnd/server.js
Outdated
|
|
||
| const app = express(); | ||
| const server = require('http').Server(app); | ||
| // const io = require('socket.io')(server); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope
| const app = express(); | ||
| const server = require('http').Server(app); | ||
| // const io = require('socket.io')(server); | ||
| var io = (module.exports.io = require('socket.io')(server)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why's this a var when all the others are consts?
src/backEnd/server.js
Outdated
|
|
||
| // module.exports = io; | ||
|
|
||
| // const express = require('express'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come on, you know how I feel about commented out code
| } | ||
| } | ||
| ${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was this block for?
| border-top-left-radius: 5px; | ||
| } | ||
| ${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these ${ } blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you get rid of these blocks? What are they for?
| @@ -0,0 +1,77 @@ | |||
| import React, { Component } from 'react'; | |||
| import FAChevronDown from 'react-icons/lib/md/keyboard-arrow-down'; | |||
| import FAMenu from 'react-icons/lib/fa/list-ul'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These aren't used so delete them
src/js/SideBar.jsx
Outdated
| const { chats, activeChat, user, setActiveChat, logout } = this.props; | ||
| return ( | ||
| <div id="side-bar"> | ||
| {/* <div className="heading"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get rid of the commented out code
| 'react-hot-loader/patch', | ||
| 'webpack-dev-server/client?http://localhost:8080', | ||
| 'webpack/hot/only-dev-server', | ||
| // 'react-hot-loader/patch', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why these have been commented out. I think they are needed.
related to #18
|
@ionush change title and PR description to relate to an issue |
| @@ -0,0 +1 @@ | |||
| DATABASE_URL= postgres://samis:12345@localhost:8080/coin | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file shouldn't be in source control.
|
|
||
| const app = express(); | ||
| const server = require('http').Server(app); | ||
| var io = (module.exports.io = require('socket.io')(server)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a const
| currentPrice: null, | ||
| differenceDollars: null, | ||
| differencePercentage: null, | ||
| monthChangeD: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should still be differenceDollars and differencePercentage throughout this file.
| border-top-left-radius: 5px; | ||
| } | ||
| ${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you get rid of these blocks? What are they for?
| module.exports = { | ||
| context: __dirname, | ||
| entry: [ | ||
| 'react-hot-loader/patch', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These shouldn't have been deleted, right?
css update and general interface improvement