AjobK / Seaqull

Seaqull programming blog
http://www.seaqull.com
3 stars 0 forks source link

312 pin comment #328

Closed danielordaan closed 2 years ago

danielordaan commented 3 years ago

closes #312


What is it supposed to do The owner of posts can pin multiple comments. Comments can also be liked (once). Comments are then ordered, first by whether they are pinned, and then on their amount of likes. image


How can we test your branch

  1. Create comments on one account
  2. Like one of them and see them ordered on refresh
  3. Create a post and comment on this post
  4. Pin some posts and repeat steps 1 and 2

To test unhappy flow, try pinning posts as a non-post-owner, or try liking comments twice


1. General checklist


2. If relevant, front-end checklist


3. If relevant, back-end checklist


4. If relevant, test these browsers


PR Rules
1. Add relevant labels.
2. Select at least two reviewers.
3. Assign all reviewers allowed to merge the branch, including yourself.
4. If relevant to the sprint, put related issue in 'Awaiting Approval' column on project board.
5. Add to the corresponding milestone.


Reviewer info
1. You can use aliases to refer to specific PR checklist items. For example 'Hey @JohnDoe, for this line please check 3.a and 1.d.'
2. Where possible, please use the suggestions feature in github so the author has a better idea of the solution you had in mind.


This form should be filled in by the author of this PR.

AjobK commented 3 years ago

Gave feedback on styling in call

AjobK commented 3 years ago

https://stackoverflow.com/a/44770035

Good to think about things like this, especially when exposing the front-end to comment IDs such as numbers. We might want to use UUIDs. What do you think?

danielordaan commented 3 years ago

https://stackoverflow.com/a/44770035

Good to think about things like this, especially when exposing the front-end to comment IDs such as numbers. We might want to use UUIDs. What do you think?

I think using UUIDs is a good idea. I think it would be best if we used both a UUID (the public id) and a regular number (the private id). With this, we can get the best of both worlds, secure public id's, and no database performance hit

danielordaan commented 3 years ago

Pushed new changes based on feedback

AjobK commented 3 years ago

https://stackoverflow.com/a/44770035 Good to think about things like this, especially when exposing the front-end to comment IDs such as numbers. We might want to use UUIDs. What do you think?

I think using UUIDs is a good idea. I think it would be best if we used both a UUID (the public id) and a regular number (the private id). With this, we can get the best of both worlds, secure public id's, and no database performance hit

I wonder if using numeric ID's is even necessary. What is the added value of using them? You have to expose an id to the front-end one way or another. Might as well just use UUIDs, which at the very least can't be used for business statistics and scrapers 🤷

When I look at this page it seems like using UUIDs instead of numeric IDs mainly has benefits. So, do you think we should completely go from numeric ID's to UUID's too? Either way we should discuss this at the next meeting.

AjobK commented 3 years ago

image

Why can't an author like someones comment?

image

EDIT: Seems like people can only like their own comment... That's a little sad 😢

AjobK commented 3 years ago

Bug display and/or description image

Order of comments is messed up?

Reproduce

danielordaan commented 3 years ago

Bug display and/or description image

Order of comments is messed up?

Reproduce

  • [ ] Add a comment
  • [ ] Add another comment
  • [ ] After waiting for 2 minutes (That's what I did) add another comment
  • [ ] Comment order is messed up!

Fixed :)

AjobK commented 3 years ago

Everything looks great! Just minimize the amount of requests as stated in the comment below. If you do let me know and we'll merge the PR to develop.

Still found a few things that should be different. Noted a few of them. Main take-aways:

  • Check your code for WET code that could be DRY.
  • Make sure that you don't overload the server, so make (if possible) singular requests that give information for a specific case.
  • Make code more atomic (functions, components) instead of directly putting things in render.
danielordaan commented 2 years ago

Generally looks good so I assume this is my last RC (Request Changes)

  1. In your branch the comments are ordered from oldest to newest. This is wrong, since the newest comments should be on top. Of course the general order should be 'Pinned > Likes > Recency'

So pinned is above unpinned, more likes is higher on the list and then you sort on recency, with the most recent comments being on top.

A nice to have would be that your own comments on a post are always on top (So if I comment on your post and I view your post I see my comments on top). If this is too much scope-creep please make it so that I see my own comment on top after sending it, but when you refresh it can be somewhere else.

  1. I don't think this is your responsibility so this will likely be a separate issue: Authors should be able to delete all comments on their posts. Not just their own comments.

Question about the nice to have: what about replies? Should your own reply also always be the first reply?

AjobK commented 2 years ago

Generally looks good so I assume this is my last RC (Request Changes)

  1. In your branch the comments are ordered from oldest to newest. This is wrong, since the newest comments should be on top. Of course the general order should be 'Pinned > Likes > Recency'

So pinned is above unpinned, more likes is higher on the list and then you sort on recency, with the most recent comments being on top. A nice to have would be that your own comments on a post are always on top (So if I comment on your post and I view your post I see my comments on top). If this is too much scope-creep please make it so that I see my own comment on top after sending it, but when you refresh it can be somewhere else.

  1. I don't think this is your responsibility so this will likely be a separate issue: Authors should be able to delete all comments on their posts. Not just their own comments.

Question about the nice to have: what about replies? Should your own reply also always be the first reply?

No, since that would mess up the 'thread' flow. Preferably you would automatically scroll down to your reply after sending your reply, but that might be out of scope. Besides, in #225 you can see that there's a bug with unfolding the replies after sending one.

AjobK commented 2 years ago
  1. Pressing pin shouldn't show a pointer or do anything when you're not the author. Now it gives an error in the console and shows a pointer.

image

image

  1. Order is still messed up... Even though I pinned and liked one of my comments (Logged in as 'User', who is also the author) I stilld don't get higher on the list. The priority should be UserComments (✔️) > Pinned (❌) > Likes (❌) > Recency (🤷)

Recency seems to work but has higher priority than we discussed in an earlier comment.

image


If this feels like too much work with all these request changes let's discuss it on Sunday. Since we don't want you to get too hung op on a task I can take over, if that helps in any way. That way we can close this PR quickly and make sure that you get the time-off that you need.

danielordaan commented 2 years ago

See comments...

We might have to talk on Sunday, since for some reason bugs pop up with every new update. This isn't necessarily a big deal, but I'm worried that you may not get the time-off you need early enough at this pace. If it helps I can take over your task.

Fixed