Skip to content

Commit

Permalink
Fix code review
Browse files Browse the repository at this point in the history
  • Loading branch information
Yang Lin committed May 27, 2020
1 parent 0d99197 commit 3a8142f
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 10 deletions.
11 changes: 3 additions & 8 deletions components/NewReplySection/index.js
@@ -1,9 +1,9 @@
import { useState, useCallback, useContext } from 'react';
import { useCallback, useContext } from 'react';
import { useMutation } from '@apollo/react-hooks';
import gql from 'graphql-tag';
import { t } from 'ttag';

import { Box, Snackbar } from '@material-ui/core';
import { Box } from '@material-ui/core';

import useCurrentUser from 'lib/useCurrentUser';
import Desktop from './Desktop';
Expand Down Expand Up @@ -63,10 +63,10 @@ const NewReplySection = withContext(
existingReplyIds,
relatedArticles,
onSubmissionComplete,
setFlashMessage,

This comment has been minimized.

Copy link
@MrOrz

MrOrz May 27, 2020

Member

I think it's rare to see parent components passing a state setter function down to its children.

  • I think NewReplySection should be only responsible for invoking event handlers like onSubmissionComplete on the right time. Since the snackbar lies in article/[id].js, it should invoke setFlashMessage when handling onSubmissionComplete .
  • It's OK for createReply and connectReply to show the same flash message "The reply has been submitted.".
  • We can add onError prop as error handler to NewReplySection, NewReplySection invoking the callback on error and article/[id].js handles it properly. However, I think it's also OK if we directly invoke alert() on error, or process errors with other means within NewReplySection.
onClose,
}) => {
const { fields, handlers } = useContext(ReplyFormContext);
const [flashMessage, setFlashMessage] = useState(0);
const currentUser = useCurrentUser();

const [createReply, { loading: creatingReply }] = useMutation(
Expand Down Expand Up @@ -148,11 +148,6 @@ const NewReplySection = withContext(
<Box display={{ xs: 'block', md: 'none' }}>
<Mobile onClose={onClose} article={article} {...sharedProps} />
</Box>
<Snackbar
open={!!flashMessage}
onClose={() => setFlashMessage('')}
message={flashMessage}
/>
</form>
);
}
Expand Down
11 changes: 9 additions & 2 deletions pages/article/[id].js
Expand Up @@ -2,7 +2,7 @@ import gql from 'graphql-tag';
import Link from 'next/link';
import { useEffect, useRef, useCallback, useState } from 'react';
import { makeStyles } from '@material-ui/core/styles';
import { Box, Divider } from '@material-ui/core';
import { Box, Divider, Snackbar } from '@material-ui/core';
import { ngettext, msgid, t } from 'ttag';

import { useRouter } from 'next/router';
Expand Down Expand Up @@ -67,7 +67,7 @@ const useStyles = makeStyles(theme => ({
},
newReplyContainer: {
position: 'fixed',
zIndex: 20,
zIndex: theme.zIndex.modal,
height: '100%',
width: '100%',
top: 0,
Expand Down Expand Up @@ -173,6 +173,7 @@ const LOAD_ARTICLE_FOR_USER = gql`
function ArticlePage() {
const { query } = useRouter();
const [showForm, setShowForm] = useState(false);
const [flashMessage, setFlashMessage] = useState(0);
const articleVars = { id: query.id };

const { data, loading } = useQuery(LOAD_ARTICLE, {
Expand Down Expand Up @@ -317,6 +318,7 @@ function ArticlePage() {
relatedArticles={article?.relatedArticles}
onSubmissionComplete={handleNewReplySubmit}
onClose={() => setShowForm(false)}
setFlashMessage={setFlashMessage}
/>
</div>
)}
Expand Down Expand Up @@ -364,6 +366,11 @@ function ArticlePage() {
)}
</div>
</div>
<Snackbar
open={!!flashMessage}
onClose={() => setFlashMessage('')}
message={flashMessage}
/>
</AppLayout>
);
}
Expand Down

0 comments on commit 3a8142f

Please sign in to comment.