Review #80
Review #80
Conversation
added protected routes
Feature: offers automatically update, with closing edits to modal on react spring and offer expiration drop down
refactor: return a value in QuestionPosted to avoid react warning
feat: add working multiple cursors
There was a problem hiding this comment.
All components are in the components folder. Maybe you can start splitting the folder, creating a containers one that contains all components connected to the store. This makes the new developer's landing a bit easier.
Also, I think the components in this project have too much logic in them. You can offload them a little bit by using some models (question) or services (apiClient).
Good job! 🚀
| case 'UPDATE_QUESTIONS': // updates questions after fetching from database | ||
| return action.questions; | ||
| case 'UPDATE_QUESTION_STATUS': // updates the question answered status in the array of questions, should be combined eventually with question as a single source of truth | ||
| let questions = [...state] |
There was a problem hiding this comment.
case 'UPDATE_QUESTION_STATUS': // updates the question answered status in the array of questions, should be combined eventually with question as a single source of truth
return state.map(question => {
if (question.question_id === action.question.question_id) {
return {
...question,
answered: true
};
} else {
return question;
}
});| </> | ||
| ) | ||
|
|
||
| const Platform = () => ( |
There was a problem hiding this comment.
const Platform = ({ match }) => (
<>
<Navbar socket={socket} landingPage={match.path === "/"} />
<Route exact path="/" component={LandingPage} />
<Route path="/ask" component={AskQuestions} />
<PrivateRoute
path="/question-posted/:questionid"
component={QuestionPosted}
/>
<PrivateRoute path="/answer" component={AnswerPage} />
<PrivateRoute path="/my-questions" component={MyQuestions} />
</>
);| } | ||
|
|
||
| renderQuestions = () => { | ||
| //User is not logged in |
There was a problem hiding this comment.
This function needs a refactor.
Maybe I'm missing something, but after some detailed reading, all this questions processing seems like you're just filtering out any question which status is not included into ['help-now', 'pending', 'closed'] and the learner is online. Which is:
this.state.questions.filter(question =>
!['help-now', 'pending', 'closed'].includes(question.status)
|| !this.state.offlineUsers.find(user => user.user_id === question.learner)
)There was a problem hiding this comment.
Anyways, all this processing in the render makes it difficult to read, try some abstractions. For example:
const questionsHelpNow = this.state.questions.filter((question) => question.isHelpNow() && question.learnerIsOffline());
is more readable than:
const questionsHelpNow = this.state.questions.filter((question) => question.status==='help-now' && this.state.offlineUsers.filter(user => user.user_id===question.learner).length===0);| } | ||
| } | ||
|
|
||
| msToTime() { |
There was a problem hiding this comment.
Generic, it doesn't belong here. Create a Time class as a helper in lib folder in the root.
|
|
||
| componentDidMount() { | ||
| this.props.socket.on('chat message', (msg) => { | ||
| let currentId = this.props.socket.id; |
There was a problem hiding this comment.
No further assignment, use const. This applies for the rest as well.
| } | ||
|
|
||
| scrollToBottom = () => { | ||
| const messages = document.getElementById('messages'); |
There was a problem hiding this comment.
Imperative again, I think this can be a good use case to use React Refs.
| overTime: 'black' | ||
| } | ||
|
|
||
| startTimer = () => { |
|
|
||
| {this.state.tutorJoined ? null : this.renderOverlay()} | ||
|
|
||
| <div className="chat-header"> |
There was a problem hiding this comment.
As this component is quite large, I recommend you not rendering any specific parts, but to delegate in other presentational components. You did it correctly with the rest of sections in this render function but not with this header.
| question: state.question, | ||
| questions: state.questions, | ||
| offers: state.offers, | ||
| tutors: state.tutors, |
| fetch(`${process.env.REACT_APP_SERVER_URL}/questions/${props.questionId}/feedback`, { | ||
| method: 'PUT', | ||
| headers : { | ||
| 'Authorization' : 'Bearer ' + token, |
There was a problem hiding this comment.
DRY, you're constructing the headers every time you need to make a fetch. Even if you are not using a middleware, you still can centralize the fetch logic into a JS module, that will works as a single instance. Take advantage of it to set the headers once you log in.
The result should look like:
apiClient.sendFeedback(karma, credits)
.then(question => props.closeQuestion(question))
…Testing merge of refactor timer
…ering fix: add extra checking for the conditional rendering of questions @ answer-page
Fixes for mobile responsive view on landing page
PR for Arol's review - Do not merge