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

"Feature: Comment Functionality Implementation" #284

Merged
merged 59 commits into from
Jun 22, 2023

Conversation

freedompraise
Copy link
Collaborator

@freedompraise freedompraise commented Jun 14, 2023

Description

This pull request adds the functionality to create and display comments for each post. Users can now engage in discussions by leaving comments on posts.

Changes Made

  • Implemented comment creation form and backend logic.
  • Integrated comment display in the post detail view.

Checklist

  • I have read the contributing guidelines
  • I have added tests to cover my changes and they all pass in addition to existing tests.
  • I have run Google Lightouse on pages where my code is used and there are no warnings or errors that are not already present in the master branch.
  • I have added documentation for my changes.

Additional Information

.gitignore Outdated Show resolved Hide resolved
ordering = ["date_posted"]

def __str__(self):
return "Comment \"{}\" {} by {}".format(self.content[3:20] , "... ",self.author)
Copy link
Owner

Choose a reason for hiding this comment

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

Knitpick...but could we use an f-string instead of .format? I believe .format() is a legacy thing.

Copy link
Owner

Choose a reason for hiding this comment

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

image

Copy link
Owner

Choose a reason for hiding this comment

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

It looks like submitting the comment does a full-page refresh (I am scrolled up to the top).

It would be cool if we made this async using HTMX...You can look at my implementation of infinite scroll to get inspiration.

I am not exactly sure how it would work in this context, but essentially, you would do something like:

<section id="create-comments-section" class="content-section">
  <h3>Make a Comment</h3>
  <form hx-post="{% url 'comment-create' %}" hx-trigger="submit" hx-swap="beforeend" hx-target="#comments-section" method="POST" action="{% url 'comment-create' %}" enctype="multipart/form-data">
    {% csrf_token %}
    {{ form.media }}
    {{ form }}
    <button class="post-submit-button" id="post-submit-button" type="submit">Comment</button>
  </form>
</section>

Copy link
Owner

@jsolly jsolly Jun 16, 2023

Choose a reason for hiding this comment

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

I think it makes more sense to just say 'Comment' rather than 'make a comment' in English. The placeholder text could be, "Leave your thoughts here..."

That's what LinkedIN does.

The text 'Content:' seems to be redundant. I suggest taking it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some reason, the htmx code doesn't work as expected. It threads the whole post under after commenting

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, I can take a stab at it after the PR merges.

@jsolly
Copy link
Owner

jsolly commented Jun 16, 2023

Hey, this is looking good! Nice work.

Doesn't look like there is a way to edit or delete comments. I am fine if we don't put those features into this PR, but we probably want to open a new issue/PR for those additional features.

Copy link
Collaborator Author

@freedompraise freedompraise left a comment

Choose a reason for hiding this comment

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

I've gone through the changes, and everything is intact.

@freedompraise
Copy link
Collaborator Author

I've gone through the changes, and everything is intact.

After you merge @jsolly, I'll create issues for editing/deleting comments, and the page scroll after commenting

@jsolly
Copy link
Owner

jsolly commented Jun 21, 2023

Hey. I sent you a PR with some changes.

I just noticed the build is failing with your PR. We will have to figure that out before we merge.

Please make sure you are using Black and Ruff on your codebase. If Ruff has any failures, it will block the build.

If you are having trouble getting those two tools setup, let me know!

@freedompraise
Copy link
Collaborator Author

Hey. I sent you a PR with some changes.

I just noticed the build is failing with your PR. We will have to figure that out before we merge.

Please make sure you are using Black and Ruff on your codebase. If Ruff has any failures, it will block the build.

If you are having trouble getting those two tools setup, let me know!

I've setup both of them, and I ran a check using black.

Copy link
Owner

Choose a reason for hiding this comment

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

Pretty sure this code is needed. I think you might have added pass when you were trying to get the initial migration working.

I suggest we put it back to the way it was.

Copy link
Owner

Choose a reason for hiding this comment

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

This needs to be removed from the PR. We don't commit the database file to source control.

@jsolly jsolly changed the base branch from master to dependabot/pip/django_project/requirements/ipython-8.14.0 June 22, 2023 00:44
@jsolly jsolly changed the base branch from dependabot/pip/django_project/requirements/ipython-8.14.0 to master June 22, 2023 00:44
@jsolly
Copy link
Owner

jsolly commented Jun 22, 2023

I made all the additional changes. Merging now!

@jsolly jsolly merged commit 24315c1 into jsolly:master Jun 22, 2023
2 of 3 checks passed
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.

2 participants