Skip to content
This repository was archived by the owner on Apr 17, 2023. It is now read-only.

Added the the ability to add comments on repositories#583

Merged
mssola merged 1 commit intoSUSE:masterfrom
jloehel:comments_2
Nov 26, 2015
Merged

Added the the ability to add comments on repositories#583
mssola merged 1 commit intoSUSE:masterfrom
jloehel:comments_2

Conversation

@jloehel
Copy link
Copy Markdown
Contributor

@jloehel jloehel commented Nov 19, 2015

Screenshots:

screenshot - 11262015 - 03 24 35 pm
screenshot - 11242015 - 05 23 29 pm
screenshot - 11242015 - 05 22 50 pm

Fixes: #204

Signed-off-by: Jürgen Löhel jloehel@suse.com

@jloehel jloehel force-pushed the comments_2 branch 2 times, most recently from 5fc944e to 82dd092 Compare November 23, 2015 13:47
@mssola
Copy link
Copy Markdown
Collaborator

mssola commented Nov 24, 2015

Good hob 👏. Now, I'm afraid that I'm going to nitpick a bit:

  • I don't like that when there are no comments, it's just a "grey" area. You can see that in your first screenshot actually. The background should be white, and there should be a text saying something on the lines of "Nobody has left a comment yet".
  • I don't quite like the icon for adding a new comment. Maybe @cyntss can help us with this. Anyways, for now you can just ignore this.
  • The "Write a Comment" link is actually not aligned with the "$count Comments" title, when it should. This is due the btn class adding a bit of extra padding. I'm not saying that you should remove this class, but maybe you could polish this. See what I'm saying in the following image:

comment

Now, I'm not so sure about the title. Honestly I would've preferred simple comments: just the text, that's it. This would make the footer of the comment unnecessary, so you could put that information on the header of the comment. Also, in the footer you put something like:

mssola commented on busybox 6 minutes ago

It's redundant to say that I commented on busybox. I'm on the busybox repo already, I know where the comment is. Moreover, since it's easy to do it, I'd also add the profile picture of the user. So, in my opinion, a comment should look like:

  • A small header (with little to no padding), no footer.
  • The header contains the profile picture, and something like "mssola commented 6 minutes ago". "mssola" should be bold and it should be a link. Just like Github.

Another think I'm not sure about is the icon being used for the activity. I'd prefer any of the 6 icons named comment in font awesome :)

Functionality-wise, the rest looks good to me. Now, let's dive into the code.

Comment thread app/assets/javascripts/comments.coffee Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't work. Mainly because it has the fa-user-plus class. Therefore, all this adding/removing of classes just doesn't work for me. Note that this would fix one of my complaints in my previous comment,

@mssola
Copy link
Copy Markdown
Collaborator

mssola commented Nov 24, 2015

Besides all my comments, in the activities you still have the problem that they are going to crash if the repo containing the added/removed comment gets removed. The way to go is:

  • Check whether the resource (trackable) actually exists.
    • If it does: use it.
    • Otherwise use what's been stored in the parameters attribute.

@jloehel jloehel force-pushed the comments_2 branch 2 times, most recently from 1d36a4f to e842196 Compare November 24, 2015 16:21
@flavio
Copy link
Copy Markdown
Member

flavio commented Nov 24, 2015

Good job @jloehel 👍

Also thanks to @mssola for the code review!

Comment thread app/models/comment.rb Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i would say it is better to compare using the user_id field, instead of building the author object which is maybe not needed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove this since the imported file is empty. And remove the app/assets/stylesheets/comments.scss file as well.
Update: irrelevant now, ignore this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants