Reviewer - diff threads #296

webdevelopland · 2018-09-12T13:05:06Z

What's new:

Diff threads are displayed on diff page
observable onDestroy bug is fixed

webdevelopland · 2018-09-12T13:06:52Z

MateMahinKhan · 2018-09-12T16:01:02Z

tools/reviewer/webapp/src/app/dashboard/diff/diff-discussion/diff-discussion.component.ts

+  // Make thread resolved/unresolved
+  toggleThread(thread: Thread): void {
+    const isDone: boolean = !thread.getIsDone();
+    thread.setIsDone(isDone);


What's happening here? something like thread.setIsDone(!thread.getIsDone()) ?
I see that you use the variable isDone later too so I guess putting it in a variable is good.

Maybe rename s/isDone/newIsDone?

I think isDone looks more natural for boolean variable.
I'll add more comments to make code more clear.

MateMahinKhan · 2018-09-12T16:02:50Z

tools/reviewer/webapp/src/app/dashboard/diff/diff-discussion/diff-discussion.component.ts

  displayedColumns = ['discussions'];
-  @Input() threads: Thread[];
+  allThreads: Thread[];


what's the difference between threads and allThreads

Yes yes, we're discussing it right now:
https://appstory.slack.com/archives/C8ZHYNGRZ/p1536759146000100

short answer: allThreads = threads + diffThreads
https://github.com/google/startup-os/blob/master/tools/reviewer/local_server/service/code_review.proto#L84

As discussed, let's rename.

oferb · 2018-09-12T17:53:25Z

tools/reviewer/webapp/src/app/dashboard/diff/diff-discussion/diff-discussion.component.ts

  displayedColumns = ['discussions'];
-  @Input() threads: Thread[];
+  allThreads: Thread[];


As discussed, let's rename.

oferb · 2018-09-12T17:55:36Z

tools/reviewer/webapp/src/app/dashboard/diff/diff-discussion/diff-discussion.component.ts

+  // Make thread resolved/unresolved
+  toggleThread(thread: Thread): void {
+    const isDone: boolean = !thread.getIsDone();
+    thread.setIsDone(isDone);


Maybe rename s/isDone/newIsDone?

oferb · 2018-09-12T17:56:45Z

tools/reviewer/webapp/src/app/dashboard/diff/diff-discussion/diff-discussion.component.ts

+  // Make thread resolved/unresolved
+  toggleThread(thread: Thread): void {
+    const isDone: boolean = !thread.getIsDone();
+    thread.setIsDone(isDone);


When we set thread.setIsDone(isDone);, how does that change get to this.diff?

thread is a link. When we edit its field, it's automatically edited in diff too.

Example:

const diff = { thread: { isDone: true } }; const thread = diff.thread; thread.isDone = false; console.log(diff.thread.isDone); // false

Ok, thanks!

oferb · 2018-09-12T18:01:11Z

tools/reviewer/webapp/src/app/dashboard/diffs/diffs.component.ts

@@ -128,7 +130,7 @@ export class DiffsComponent implements OnInit {
  sortDiffs(): void {
    for (const diffList of this.diffGroups) {
      diffList.sort((a, b) => {
-        // Newest first
+        // Newest on the top


s/on the top/on top

oferb · 2018-09-14T11:36:12Z

tools/reviewer/webapp/README.md

+[node](https://nodejs.org/) LTS  
+[npm](https://www.npmjs.com/)  
+[protoc](https://github.com/protocolbuffers/protobuf/releases)  
+Optional:  


What do the optionals allow?

firebase allow to deploy.
angular allow to use ng features. For example:

ng lint -fix

or

ng generate component my-component

You think we should add the information to the README?

Just mention it briefly, so people know if they need it or not.

oferb · 2018-09-14T11:36:52Z

tools/reviewer/webapp/README.md

@@ -1,13 +1,21 @@
+## You need installed
+[node](https://nodejs.org/) LTS  


Which versions? :)

Can you mention which versions currently work for you?

I'm not sure it's necessary. No matter which version we use, until it's LTS everything is ok.
Also, LTS is updated all the time. It may be annoying to update the information. We can just refer to last LTS on nodejs site.

tools/reviewer/webapp/README.md

oferb · 2018-09-14T11:41:43Z

tools/reviewer/webapp/src/app/dashboard/diff/diff-discussion/diff-discussion.component.ts

 import { DiffService } from '../diff.service';

+// To distinguish diff threads and line threads
+interface ThreadFrame {


We can add a boolean field to Thread.

oferb · 2018-09-15T12:10:05Z

tools/reviewer/webapp/README.md

@@ -1,13 +1,21 @@
+## You need installed
+[node](https://nodejs.org/) LTS  


Can you mention which versions currently work for you?

oferb · 2018-09-15T12:10:32Z

tools/reviewer/webapp/README.md

+[node](https://nodejs.org/) LTS  
+[npm](https://www.npmjs.com/)  
+[protoc](https://github.com/protocolbuffers/protobuf/releases)  
+Optional:  


Just mention it briefly, so people know if they need it or not.

oferb · 2018-09-15T12:11:18Z

tools/reviewer/local_server/service/code_review.proto

@@ -78,7 +78,8 @@ message Diff {
  string workspace = 8;
  int64 created_timestamp = 9;
  int64 modified_timestamp = 10;
-  repeated Thread thread = 13;
+  // Threads on a line.
+  repeated Thread line_thread = 13;


After some more thought, I think we need an enum: #323
Feel free to copy the changes into your PR.

diff threads

04e3383

webdevelopland requested review from MateMahinKhan and oferb September 12, 2018 13:05

oferb mentioned this pull request Sep 12, 2018

Display diff threads #300

Closed

MateMahinKhan reviewed Sep 12, 2018

View reviewed changes

MateMahinKhan approved these changes Sep 12, 2018

View reviewed changes

oferb suggested changes Sep 12, 2018

View reviewed changes

review fix 1

9c0522a

webdevelopland mentioned this pull request Sep 14, 2018

Fix reviewer webapp README #302

Closed

Alternative way to distinguish threads

b770c05

oferb suggested changes Sep 14, 2018

View reviewed changes

review fix 2

5775c21

oferb suggested changes Sep 15, 2018

View reviewed changes

webdevelopland added 2 commits September 15, 2018 19:45

review fix 3

ae9ea2f

Merge branch 'master' into WD18

857185f

oferb approved these changes Sep 15, 2018

View reviewed changes

webdevelopland added 2 commits September 15, 2018 19:54

remove break

0c40228

README

1c7699f

oferb approved these changes Sep 15, 2018

View reviewed changes

LTS version

e7fce3a

webdevelopland merged commit 2223e5a into master Sep 15, 2018

oferb deleted the WD18 branch September 15, 2018 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reviewer - diff threads #296

Reviewer - diff threads #296

webdevelopland commented Sep 12, 2018 •

edited

Loading

webdevelopland commented Sep 12, 2018

MateMahinKhan Sep 12, 2018

oferb Sep 12, 2018

webdevelopland Sep 13, 2018

MateMahinKhan Sep 12, 2018

webdevelopland Sep 12, 2018 •

edited

Loading

oferb Sep 12, 2018

oferb Sep 12, 2018

oferb Sep 12, 2018

oferb Sep 12, 2018

webdevelopland Sep 12, 2018

webdevelopland Sep 13, 2018 •

edited

Loading

oferb Sep 14, 2018

oferb Sep 12, 2018

oferb Sep 14, 2018

webdevelopland Sep 15, 2018 •

edited

Loading

webdevelopland Sep 15, 2018

oferb Sep 15, 2018

oferb Sep 14, 2018

webdevelopland Sep 15, 2018

oferb Sep 15, 2018

webdevelopland Sep 15, 2018 •

edited

Loading

oferb Sep 14, 2018

oferb Sep 15, 2018

oferb Sep 15, 2018

oferb Sep 15, 2018

		@@ -1,13 +1,21 @@
		## You need installed
		[node](https://nodejs.org/) LTS

Reviewer - diff threads #296

Reviewer - diff threads #296

Conversation

webdevelopland commented Sep 12, 2018 • edited Loading

webdevelopland commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webdevelopland Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webdevelopland Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webdevelopland Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webdevelopland Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webdevelopland commented Sep 12, 2018 •

edited

Loading

webdevelopland Sep 12, 2018 •

edited

Loading

webdevelopland Sep 13, 2018 •

edited

Loading

webdevelopland Sep 15, 2018 •

edited

Loading

webdevelopland Sep 15, 2018 •

edited

Loading