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

Add Feature Edit Post #118

Open
wants to merge 7 commits into
base: v1
Choose a base branch
from
Open

Add Feature Edit Post #118

wants to merge 7 commits into from

Conversation

rkreddy99
Copy link

@rkreddy99 rkreddy99 commented May 16, 2021

Users can edit their post by clicking on edit icon for their posts.

@changeset-bot
Copy link

changeset-bot bot commented May 16, 2021

⚠️ No Changeset found

Latest commit: 55911c1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented May 16, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/hoangvvo/nextjs-mongodb-app/8oEopmVKAZQWdBHsMt4ogFzKWZpE
✅ Preview: https://nextjs-mongodb-app-git-fork-rkreddy99-master-hoangvvo.vercel.app

@rkreddy99
Copy link
Author

rkreddy99 commented May 17, 2021

This pull request has two commits Add Feature Edit Post and Add Edit Post Feature.

Add Feature Edit Post

  • Edit the post in Post component from post.jsx

Add Edit Post Feature

  • Edit the post using PostEditor component from editor.jsx

Merge the commit based on the requirement.

@ItaiAxelrad
Copy link
Contributor

Please be sure to check your editor and prettier config and align it with the one used for this project (single/double quotes, indents, etc.). Otherwise you end up with additional changes that make your commit harder to review.

@rkreddy99
Copy link
Author

rkreddy99 commented May 22, 2021

@ItaiAxelrad Can you tell me how to get the original prettier configuration.
Before:
image
After: (my prettier configuration)
image

or is it fine to use my prettier configuration.

@hoangvvo
Copy link
Owner

Do you install prettier on your machine? @rkreddy99 If so try disabling and use eslint do to code formatting.

@rkreddy99
Copy link
Author

I wasn't able to format the code using eslint, but still tried to be consistent using prettier. @hoangvvo Is it ok?

@hoangvvo
Copy link
Owner

You can try running

npx eslint . --fix

@rkreddy99
Copy link
Author

You can try running

npx eslint . --fix

Done. @hoangvvo

@rkreddy99
Copy link
Author

Does anything more needed to be added in this commit @hoangvvo @ItaiAxelrad

<small>{new Date(post.createdAt).toLocaleString()}</small>
{user?._id === currUser?._id && (
Copy link
Owner

Choose a reason for hiding this comment

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

In this case, what if both user and currUser is undefined?

}

const discard = () => {
makeEdit();
Copy link
Owner

Choose a reason for hiding this comment

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

This call is not clear what it does.

Comment on lines +11 to +21
const [edit, setEdit] = useState(false);
const [content, setContent] = useState(post.content);
const [msg, setMsg] = useState('');
const user = useUser(post.creatorId);
const [currUser] = useCurrentUser();
const makeEdit = (msg, text) => {
setEdit(false);
setMsg(msg);
setTimeout(() => setMsg(null), 1500);
if (text) setContent(text);
};
Copy link
Owner

Choose a reason for hiding this comment

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

I think this logic should be inside PostEditor. Howabout passing an id into it. something like:

<PostEditor editingId="foo" />

}
small {
color: #777;
}
`}
</style>
<div>
<div style={{ position: 'relative' }}>
<p style={{ color: '#0070f3', textAlign: 'center' }}>{msg}</p>
Copy link
Owner

Choose a reason for hiding this comment

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

This should be inside PostEditor similar to the other comment

@hoangvvo
Copy link
Owner

hoangvvo commented Jun 8, 2021

Does anything more needed to be added in this commit @hoangvvo @ItaiAxelrad

I am really sorry. I have been really busy lately, but I just added some comments.

Thank you.

Comment on lines -49 to 124
return useSWRInfinite((index, previousPageData) => {
// reached the end
if (previousPageData && previousPageData.posts.length === 0) return null;
return useSWRInfinite(
(index, previousPageData) => {
// reached the end
if (previousPageData && previousPageData.posts.length === 0) return null;

// first page, previousPageData is null
if (index === 0) {
return `/api/posts?limit=${PAGE_SIZE}${
creatorId ? `&by=${creatorId}` : ''
}`;
}
// first page, previousPageData is null
if (index === 0) {
return `/api/posts?limit=${PAGE_SIZE}${creatorId ? `&by=${creatorId}` : ''}`;
}

// using oldest posts createdAt date as cursor
// We want to fetch posts which has a datethat is
// before (hence the .getTime() - 1) the last post's createdAt
const from = new Date(
new Date(
previousPageData.posts[previousPageData.posts.length - 1].createdAt,
).getTime() - 1,
).toJSON();
// using oldest posts createdAt date as cursor
// We want to fetch posts which has a datethat is
// before (hence the .getTime() - 1) the last post's createdAt
const from = new Date(
new Date(previousPageData.posts[previousPageData.posts.length - 1].createdAt).getTime() - 1
).toJSON();

return `/api/posts?from=${from}&limit=${PAGE_SIZE}${
creatorId ? `&by=${creatorId}` : ''
}`;
}, fetcher, {
refreshInterval: 10000, // Refresh every 10 seconds
});
return `/api/posts?from=${from}&limit=${PAGE_SIZE}${creatorId ? `&by=${creatorId}` : ''}`;
},
fetcher,
{
refreshInterval: 10000, // Refresh every 10 seconds
}
);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Lots of these changes are stylistic changes (maybe because of some formatting tool on your local), which make it difficult to review.

Is it possible to undo these?

Copy link
Author

Choose a reason for hiding this comment

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

Will take care of it.

Comment on lines +141 to +150
<button
type="button"
style={{
background: 'transparent',
color: '#000',
}}
onClick={() => setSize(size + 1)}
disabled={isReachingEnd || isLoadingMore}>
{isLoadingMore ? '. . .' : 'load more'}
</button>
Copy link
Owner

Choose a reason for hiding this comment

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

Let's try avoid stylistic changes

Copy link
Author

Choose a reason for hiding this comment

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

ok

Comment on lines +137 to +139
{posts.map((post) => (
<Post key={post._id} post={post} />
))}
Copy link
Owner

Choose a reason for hiding this comment

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

Let's try avoid stylistic changes

Comment on lines -21 to +44
return db.collection('posts').insertOne({
_id: nanoid(12),
content,
creatorId,
createdAt: new Date(),
}).then(({ ops }) => ops[0]);
return db
.collection('posts')
.insertOne({
_id: nanoid(12),
content,
creatorId,
createdAt: new Date(),
})
.then(({ ops }) => ops[0]);
Copy link
Owner

Choose a reason for hiding this comment

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

Let's try avoid stylistic changes

@@ -28,6 +28,7 @@ handler.get(async (req, res) => {
if (!req.user) return res.json({ user: null });
const { password, ...u } = req.user;
res.json({ user: u });
return null;
Copy link
Owner

Choose a reason for hiding this comment

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

Is it required?

Comment on lines +37 to +40
const post = await editPost(req.db, {
content: req.body.content,
postId: req.body.postId,
});
Copy link
Owner

Choose a reason for hiding this comment

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

We need to authenticate this. Cannot let a user edit other people's posts

Copy link
Contributor

@ItaiAxelrad ItaiAxelrad left a comment

Choose a reason for hiding this comment

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

Will review more thoroughly once style conforms and existing requested changes have been made.

import { defaultProfilePicture } from '@/lib/default';
import PostEditor from './editor';
Copy link
Contributor

Choose a reason for hiding this comment

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

change to import PostEditor from '@/components/post/editor'; per jsconfig

<p style={{ color: '#0070f3', textAlign: 'center' }}>{msg}</p>
<form
onSubmit={hanldeSubmit}
onReset={discard}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not abstract the logic too much, change to: onReset={() => setEdit(false)}

}

const discard = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove discard in favor of passing in setEdit as prop

@@ -1,51 +1,88 @@
import React, { useState } from 'react';
import { useCurrentUser } from '@/hooks/index';

export default function PostEditor() {
export default function PostEditor({ edit, makeEdit, text, Id }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add setEdit as prop: export default function PostEditor({ edit, makeEdit, setEdit, text, Id })

{post.content}
</p>
{edit === true ? (
<PostEditor edit={edit} makeEdit={makeEdit} text={content} Id={post._id} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Add setEdit={setEdit} to props

content: e.currentTarget.content.value,
postId: Id,
};
// if (!e.currentTarget.content.value) return;

Choose a reason for hiding this comment

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

Commented code should not be pushed.

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.

4 participants