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

Discussions (a.k.a. Grouped Comments) #1878

Merged
merged 47 commits into from Jan 15, 2013

Conversation

7 participants
@riyad
Contributor

riyad commented Oct 31, 2012

This revamps the merge request comments section to group related comments (on commits and diffs) and shows a little context.
There is also a lot of tidying up, fixing subtle deficiencies, inconsistencies and a few bugs.

  • group comments based on commit, commit diff line, merge request diff line
  • show context of embedded discussions
  • make discussions collapsible
  • fix icons and text for diff line links/buttons/form to be more consistent
  • truncate diff context only to the relevant parts
  • continuous loading only for the wall
  • update styling for note actions
  • allow previews and attachments everywhere
  • improve attachments
  • simplifying and cleaning CSS and JS

This is based on code, ideas and discussions from #1733. @kouno thanks again. :)

@ghost ghost assigned riyad Oct 31, 2012

@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Oct 31, 2012

Contributor

Preliminary screen shots:

an embedded discussion

embedded discussions say where they actually come from and when they have been updated. also you can hide them if you want

a collapsed discussion

changed mr tabs

icons and text changed

diff line form and links

everything is consistently called "comment", also the reply icon is now on the button

comment actions

comment actions are greyed out and appear only if you hover over the comment.
if you hover over the action, it will glow with color (e.g. blue). the delete icon "glows" red, when you hover over it

Contributor

riyad commented Oct 31, 2012

Preliminary screen shots:

an embedded discussion

embedded discussions say where they actually come from and when they have been updated. also you can hide them if you want

a collapsed discussion

changed mr tabs

icons and text changed

diff line form and links

everything is consistently called "comment", also the reply icon is now on the button

comment actions

comment actions are greyed out and appear only if you hover over the comment.
if you hover over the action, it will glow with color (e.g. blue). the delete icon "glows" red, when you hover over it

@dzaporozhets

This comment has been minimized.

Show comment
Hide comment
@dzaporozhets

dzaporozhets Oct 31, 2012

Member

👍

it a really useful stuff

Member

dzaporozhets commented Oct 31, 2012

👍

it a really useful stuff

@jozefvaclavik

This comment has been minimized.

Show comment
Hide comment
@jozefvaclavik

jozefvaclavik Nov 4, 2012

Contributor

Just short question: are you planning to implement Notes api for Issues as well?

Contributor

jozefvaclavik commented Nov 4, 2012

Just short question: are you planning to implement Notes api for Issues as well?

@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Nov 6, 2012

Contributor

@jozefvaclavik This has nothing to do with the Notes API. It is a new approach to how things are presented through the browser.

Contributor

riyad commented Nov 6, 2012

@jozefvaclavik This has nothing to do with the Notes API. It is a new approach to how things are presented through the browser.

@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Nov 21, 2012

Contributor

Update:

  • huge code refactoring
  • discussions and diff notes now share many partials and almost all JS
  • diff notes look a lot nicer
  • you can now have many reply forms open
  • no paging for ordinary commits any more ("infinite scroll" only supported for the wall)

Known Issues:

  • styling for the note forms in discussion/diff lines is off sometimes
  • note form on the wall is broken
Contributor

riyad commented Nov 21, 2012

Update:

  • huge code refactoring
  • discussions and diff notes now share many partials and almost all JS
  • diff notes look a lot nicer
  • you can now have many reply forms open
  • no paging for ordinary commits any more ("infinite scroll" only supported for the wall)

Known Issues:

  • styling for the note forms in discussion/diff lines is off sometimes
  • note form on the wall is broken
@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Nov 22, 2012

Contributor

Update:

  • fixed preview
  • fixed wall
  • started playing with improving attachment indications
Contributor

riyad commented Nov 22, 2012

Update:

  • fixed preview
  • fixed wall
  • started playing with improving attachment indications
@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Nov 22, 2012

Contributor

new diff note style:

Contributor

riyad commented Nov 22, 2012

new diff note style:

@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Nov 22, 2012

Contributor

reply everywhere (at once!)

Contributor

riyad commented Nov 22, 2012

reply everywhere (at once!)

@dzaporozhets

This comment has been minimized.

Show comment
Hide comment
@dzaporozhets

dzaporozhets Nov 22, 2012

Member

wow! Cool

Member

dzaporozhets commented Nov 22, 2012

wow! Cool

@SaitoWu

This comment has been minimized.

Show comment
Hide comment
@SaitoWu

SaitoWu Nov 22, 2012

Contributor

@riyad Awesome!

Contributor

SaitoWu commented Nov 22, 2012

@riyad Awesome!

@vsizov

This comment has been minimized.

Show comment
Hide comment
@vsizov

vsizov Nov 22, 2012

Contributor

cool

Contributor

vsizov commented Nov 22, 2012

cool

@dzaporozhets

This comment has been minimized.

Show comment
Hide comment
@dzaporozhets

dzaporozhets Nov 27, 2012

Member

Its in progress yeap?

Member

dzaporozhets commented Nov 27, 2012

Its in progress yeap?

@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Dec 3, 2012

Contributor

Update:
Generalized a lot of form handling code. There is a lot less difference between main and discussion note forms.

  • preview everywhere
  • form options (e.g. notifications, attachment) everywhere
  • form errors (when necessary) everywhere
  • lots of CSS cleanup
Contributor

riyad commented Dec 3, 2012

Update:
Generalized a lot of form handling code. There is a lot less difference between main and discussion note forms.

  • preview everywhere
  • form options (e.g. notifications, attachment) everywhere
  • form errors (when necessary) everywhere
  • lots of CSS cleanup
@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Dec 3, 2012

Contributor

new form errors:

Contributor

riyad commented Dec 3, 2012

new form errors:

@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Dec 3, 2012

Contributor

preview and options everywhere (these weren't available for diff/discussion notes before):

Contributor

riyad commented Dec 3, 2012

preview and options everywhere (these weren't available for diff/discussion notes before):

@dzaporozhets

This comment has been minimized.

Show comment
Hide comment
@dzaporozhets

dzaporozhets Dec 4, 2012

Member

wow 😄

Member

dzaporozhets commented Dec 4, 2012

wow 😄

@kouno

This comment has been minimized.

Show comment
Hide comment
@kouno

kouno Dec 4, 2012

Contributor

Absolutely ridiculous. I love it 👍

And there is more than 2 000 lines of changes. Are you aiming for 9001? :)

Contributor

kouno commented Dec 4, 2012

Absolutely ridiculous. I love it 👍

And there is more than 2 000 lines of changes. Are you aiming for 9001? :)

riyad added some commits Dec 23, 2012

Merge branch 'master' into discussions
Conflicts:
	app/assets/stylesheets/main.scss
	app/models/project.rb
	app/views/notes/_common_form.html.haml
	app/views/notes/_per_line_form.html.haml
	lib/gitlab/markdown.rb
	spec/models/note_spec.rb
@ascii-soup

This comment has been minimized.

Show comment
Hide comment
@ascii-soup

ascii-soup Jan 14, 2013

We're really looking forward to this PR as it will remove a number of the issues we have with GitLab when reviewing MRs. Is this still on track for 4.1?

ascii-soup commented Jan 14, 2013

We're really looking forward to this PR as it will remove a number of the issues we have with GitLab when reviewing MRs. Is this still on track for 4.1?

@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Jan 14, 2013

Contributor

@ascii-soup It's still being worked on. Hammering down the last bugs with tests and in the tests. ;)

Contributor

riyad commented Jan 14, 2013

@ascii-soup It's still being worked on. Hammering down the last bugs with tests and in the tests. ;)

riyad added some commits Jan 14, 2013

Merge commit 'master' into discussions
Conflicts:
	app/assets/stylesheets/sections/notes.scss
	app/contexts/notes/load_context.rb
	app/models/project.rb
	app/observers/note_observer.rb
	app/roles/votes.rb
	app/views/commit/show.html.haml
	app/views/merge_requests/_show.html.haml
	app/views/merge_requests/diffs.js.haml
	app/views/merge_requests/show.js.haml
	app/views/notes/_note.html.haml
	features/steps/project/project_merge_requests.rb
	spec/models/note_spec.rb
@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Jan 14, 2013

Contributor

@randx My spinach tests are failing when they require a proper Ajax response. The (functionally) same tests worked when I wrote them as rspec request specs. Any idea what I need to fix to make them work?

Contributor

riyad commented Jan 14, 2013

@randx My spinach tests are failing when they require a proper Ajax response. The (functionally) same tests worked when I wrote them as rspec request specs. Any idea what I need to fix to make them work?

@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Jan 15, 2013

Contributor

Rebases have become to cumbersome ... from now on there are merges with the current master branch.

  • Merged master
  • Fixed to work with recent changes in the Note model (i.e. the commit_id stuff)
  • Rewrote my Rspec request specs as Spinach features (still missing some)
Contributor

riyad commented Jan 15, 2013

Rebases have become to cumbersome ... from now on there are merges with the current master branch.

  • Merged master
  • Fixed to work with recent changes in the Note model (i.e. the commit_id stuff)
  • Rewrote my Rspec request specs as Spinach features (still missing some)
@dzaporozhets

This comment has been minimized.

Show comment
Hide comment
@dzaporozhets

dzaporozhets Jan 15, 2013

Member

I will review and test it today

Member

dzaporozhets commented Jan 15, 2013

I will review and test it today

@dzaporozhets

This comment has been minimized.

Show comment
Hide comment
@dzaporozhets

dzaporozhets Jan 15, 2013

Member

I made few fixes and improvements and will merge it.

Next time we should avoid such huge Pull Requests.

@riyad instead project.add_access(@user, :read, :write) we now use project.tead << [@user, :developer]

also @riyad thank you for such huge work

Member

dzaporozhets commented Jan 15, 2013

I made few fixes and improvements and will merge it.

Next time we should avoid such huge Pull Requests.

@riyad instead project.add_access(@user, :read, :write) we now use project.tead << [@user, :developer]

also @riyad thank you for such huge work

@dzaporozhets dzaporozhets merged commit bda7fe3 into gitlabhq:master Jan 15, 2013

1 check failed

default The Travis build failed
Details
@ascii-soup

This comment has been minimized.

Show comment
Hide comment
@ascii-soup

ascii-soup Jan 15, 2013

👍 Really excited for this!

ascii-soup commented Jan 15, 2013

👍 Really excited for this!

@riyad

This comment has been minimized.

Show comment
Hide comment
@riyad

riyad Jan 15, 2013

Contributor

@randx thanks for the fixes. :)

Contributor

riyad commented Jan 15, 2013

@randx thanks for the fixes. :)

@dzaporozhets

This comment has been minimized.

Show comment
Hide comment
@dzaporozhets

dzaporozhets Jan 15, 2013

Member

@riyad np. still 8 specs left to fix but its nothing. Guess it's a last big feature for 4.1

Member

dzaporozhets commented Jan 15, 2013

@riyad np. still 8 specs left to fix but its nothing. Guess it's a last big feature for 4.1

@riyad riyad deleted the riyad:discussions branch Jan 15, 2013

dzaporozhets added a commit that referenced this pull request Mar 29, 2015

Merge branch 'fix-plain-diffs' into 'master'
Remove the email patches link for merge commits

Rugged's `to_mbox` method doesn't work for merge commits, so don't display the option to download them as email patches.  This is part of the fix for #1878, and gitlab/gitlab_git!27 allows a plain diff to be generated for a merge commit.  Together these changes should prevent the 500 errors described in the issue.

cc @sytse

See merge request !1737
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment