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

View linter messages in the compare view for versions #314

Closed
kumar303 opened this issue Mar 4, 2019 · 4 comments · Fixed by #417
Closed

View linter messages in the compare view for versions #314

kumar303 opened this issue Mar 4, 2019 · 4 comments · Fixed by #417

Comments

@kumar303
Copy link
Contributor

kumar303 commented Mar 4, 2019

User story:

As a code reviewer, I want to see linter messages shown in the version compare view to speed up the review process.

This will be similar to showing linter messages in the file viewer: #41

Example from the current site:

screen shot 2019-03-04 at 3 00 03 pm

@kumar303
Copy link
Contributor Author

kumar303 commented Mar 4, 2019

We will need a way to link into the validation.json URL: mozilla/addons#6288

@kumar303
Copy link
Contributor Author

kumar303 commented Mar 6, 2019

The linter messages shown will be for the newer version in the compare view

@willdurand
Copy link
Member

In the DiffView component, one can show inlined content using getWidgets():

+  getWidgets = (hunks) => {
+    return (
+      this.state.changesWithComments
+        .reduce((widgets, change) => {
+          const changeKey = getChangeKey(change);
+
+          return {
+            ...widgets,
+            [changeKey]: (
+              <Comment
+                comment={
+                  change.lineNumber === 39
+                    ? null
+                    : { content: 'This is not needed.' }
+                }
+              />
+            ),
+          };
+        }, {})
+    );
+  };

It also needs widgets={this.getWidgets(hunks)} to each Diff in render().

The diff above ha a list of comments stored in this.state.changesWithComments because a button was also added to add new comments. For linter results, we won't need the state, but it shows how to create a list of widgets. The lineNumber can be found via change.lineNumber but it could also be change.oldLineNumber or change.newLineNumber.

@kumar303
Copy link
Contributor Author

I think we added needs: api because we wanted the results included in the main API response (mozilla/addons#6380) but we have a workaround in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants