[Code] diff page #46714

WangQianliang · 2019-09-26T15:08:52Z

Summary

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes [i18n support]~~(https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
~~- [ ] Documentation was added for features that require explanation or tutorials~~
~~- [ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-09-26T15:08:54Z

Pinging @elastic/code

elasticmachine · 2019-09-26T16:21:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-10-08T07:49:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 97ea9cdd4028ae7ce3467951220d164cc04afab3

elasticmachine · 2019-10-10T03:52:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 4031211b070496bfd0608cb9997ac9f9670ac945

elasticmachine · 2019-10-10T15:14:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 3d80a34906e8141fda2a4e15f6e3bc0c0cadb044

elasticmachine · 2019-10-10T16:00:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: c193e924671a7b6ae29e31657dfafb638b598aa8

elasticmachine · 2019-10-11T07:12:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c11c223414e3358722fccae9cf3e6e3ab1e7b8b7

elasticmachine · 2019-10-14T03:50:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ab64592c80dd8a099a813a896f0baf033ea6345d

elasticmachine · 2019-10-14T16:17:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a2a779305e085fe7cf2b851e24475adec94bf0a8

elasticmachine · 2019-10-15T04:19:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: caea17354eec9887d25f837cb310172b08202bf2

elasticmachine · 2019-10-15T06:05:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d5c901917b95ad069a4285e89651c15136ab9b42

WangQianliang · 2019-10-15T13:32:15Z

@mw-ding @daveyholler @rylnd Please take a look

elasticmachine · 2019-10-15T13:56:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4a29f81eae4c1b2e196b8377637092516d26fb33

mw-ding

Mostly good. A couple of comments below:

too much blank space for small files
I know it's intended to rendering diff in monaco editor to be able to scroll horizontally and vertically, but it's a bit weird to behave in this way. Not a blocking issue for this PR, but let's confirm on our next week's design sync.
@daveyholler should we have different styles for the commit message area?

I feel like these 2 lines should be differentiated more here.

rylnd

Besides my inline comments, I've got a few broad style comments, mostly echoing what @mw-ding said:

text feels a little cramped
colors are inconsistent, e.g. +++/--- are not the same hues as the line highlights and so aren't strongly correlated
Do we want/need the minimap here? It feels like noise to me, and I don't think any other monaco instances use it.
There's a lot of gray and it's sort of distracting. Part of it's the minimap, but even the striped background is a little dark to me.

I'll defer to @daveyholler for the final say on style stuff, of course.

rylnd · 2019-10-15T21:14:21Z

x-pack/legacy/plugins/code/public/components/commits/commit.tsx

@@ -55,7 +54,7 @@ const revisionLinkLabel = i18n.translate('xpack.code.commits.revisionLinkAriaLab
 });

 const CommitActions = ({ commitId, repoUri }: ActionProps) => {
-  const revisionPath = `#/${repoUri}/${PathTypes.tree}/${commitId}`;
+  const revisionPath = `#/${repoUri}/commit/${commitId}`;


Should we add a PathType for this new path, or is that stuff going away? I know we've been alluding to some more robust path-generation helpers, but I'm not sure where we landed.

commit is not one of the PathTypes

So what is the purpose of PathTypes? I would have thought that this new diff page would be represented in that enum, unless PathTypes itself is deprecated.

PathTypes defines different path types in source view page whose URIs share the same structure as /$repoUri/.../$filePath, while diff page is a different page.

Got it, thanks!

I would suggest to add a frontend flag (e.g. enableDiffPage) in here with default value false before we expose this page into the main app.

x-pack/legacy/plugins/code/public/components/commits/commit_link.tsx

x-pack/legacy/plugins/code/public/components/diff_page/diff.scss

elasticmachine · 2019-10-16T13:28:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cce066ceaf023ba78417c0c8b3521e4f2ddc4f8f

WangQianliang · 2019-10-16T14:55:12Z

This red area is a diff overview, sadly I didn't find a way to remove this.

elasticmachine · 2019-10-16T16:27:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 1fec27e71e940916500fd56b87220dadb98bd943

rylnd

@WangQianliang a few more things here:

Please try not to alter the commit history during a review; it makes it very hard to determine what has changed since the reviewer last saw things.
Header still needs to call out the commit message better; maybe just more font-weight and some bottom margin? (cc @daveyholler)
The red/green colors should be consistent. I would find two colors in EUI, and use those with the lighten function in our CSS.
some minor rebase issues

rylnd · 2019-10-16T20:06:28Z

x-pack/legacy/plugins/code/public/components/commits/commit.tsx

@@ -55,7 +54,7 @@ const revisionLinkLabel = i18n.translate('xpack.code.commits.revisionLinkAriaLab
 });

 const CommitActions = ({ commitId, repoUri }: ActionProps) => {
-  const revisionPath = `#/${repoUri}/${PathTypes.tree}/${commitId}`;
+  const revisionPath = `#/${repoUri}/commit/${commitId}`;


So what is the purpose of PathTypes? I would have thought that this new diff page would be represented in that enum, unless PathTypes itself is deprecated.

rylnd · 2019-10-16T22:03:41Z

x-pack/legacy/plugins/code/public/monaco/monaco_diff_editor.ts

@@ -26,6 +26,21 @@ export class MonacoDiffEditor {
        enableSplitViewResizing: false,
        renderSideBySide: this.renderSideBySide,
        scrollBeyondLastLine: false,
+        readOnly: true,
+        minimap: {


I also had no luck hiding the overview. However, it might be worth adding overviewRulerBorder: false to cut down slightly on the noise that it adds.

rylnd · 2019-10-16T22:16:35Z

x-pack/legacy/plugins/code/public/monaco/override_monaco_styles.scss

+
+  .monaco-diff-editor .line-insert,
+  .monaco-diff-editor .char-insert {
+    background-color: rgba(0, 179, 164, 0.1);


You should use the lighten function on an $euiColor variable here, to make its origin and intention more apparent.

Also, the deletion color is much more purple than red (I think you're just using the default EuiBadge color?), and if we're overriding one to be green we might as well override the other to be red.

rylnd · 2019-10-16T22:17:50Z

x-pack/legacy/plugins/code/public/style/_monaco.scss

 }

 .codeBlock__line--highlighted {
  background-color: $euiColorLightShade;
 }

+.code-monaco-highlight-line {


These styles were deleted on master, I don't think you want to re-add them here. Same with .code-mark-line-number

elasticmachine · 2019-10-17T04:08:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: e3947c1b874014eac0253c110e04a9fa5665587f

elasticmachine · 2019-10-17T14:19:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ed139cd58b2f72c4734eb15b41aa76bdb51f9fef

daveyholler

I opened up a PR against your branch. https://github.com/WangQianliang/kibana/pull/1

I agree with Ryland on the styling in the actual diff panels. We should bring those colors more in line with the colors used on the badges. In addition to the lighten() SCSS method, there's also a tintOrShade() that you can use from EUI that could help get there.

Regarding that overview, I was able to hide it, but it leaves empty space in its place which is arguably worse. 🤷‍♂

rylnd · 2019-10-17T16:22:18Z

Regarding that overview, I was able to hide it, but it leaves empty space in its place which is arguably worse. 🤷‍♂

@daveyholler How did you do it?! I could not figure out a configuration that did so.

WangQianliang · 2019-10-18T01:03:54Z

Regarding that overview, I was able to hide it, but it leaves empty space in its place which is arguably worse. 🤷‍♂

@daveyholler How did you do it?! I could not figure out a configuration that did so.

Override the css to display: none would do.

elasticmachine · 2019-10-18T02:37:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6db1654832a32841a53c53c656290f7730e06634

rylnd · 2019-10-18T17:25:25Z

Override the css to display: none would do.

Oh, well, that's cheating 😜

rylnd

LGTM!

mw-ding

Approved to move this PR forward. As I mentioned in the comment, please add a frontend flag with default value false to control if the commit message will link to this Diff page, mostly because it's still not 100% sure if we would like to release the current diff view component based on Monaco editor.

mw-ding · 2019-10-18T22:57:06Z

x-pack/legacy/plugins/code/server/git_operations.ts

@@ -318,7 +318,7 @@ export class GitOperations {
    const git = await this.openGit(uri);
    const commit = await this.getCommitOr404(uri, revision);
    if (!revision.includes('..')) {
-      revision = `${revision}..${revision}~1`;
+      revision = `${revision}~1..${revision}`;


can you double check on this?

Strictly speaking, doesn’t this just compare the commit to its first parent? If it’s a squashed PR that will be pre-merge master, but I can imagine a lot of cases where that wouldn’t be the case.

If we truly want to compare to master, we should explicitly use that, or do something a little fancier with git merge-base

mw-ding · 2019-10-18T22:59:07Z

x-pack/legacy/plugins/code/public/components/commits/commit.tsx

@@ -55,7 +54,7 @@ const revisionLinkLabel = i18n.translate('xpack.code.commits.revisionLinkAriaLab
 });

 const CommitActions = ({ commitId, repoUri }: ActionProps) => {
-  const revisionPath = `#/${repoUri}/${PathTypes.tree}/${commitId}`;
+  const revisionPath = `#/${repoUri}/commit/${commitId}`;


I would suggest to add a frontend flag (e.g. enableDiffPage) in here with default value false before we expose this page into the main app.

elasticmachine · 2019-10-19T11:15:00Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 212360e3373f48b0023c971d067e53ef3ed9e2ed

elasticmachine · 2019-10-20T13:05:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1afbc1e4ea6c2057689e3891d27071c5d621c2e6

elasticmachine · 2019-10-21T04:35:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ae1c6b4

* fix(code/frontend): enable diff page

WangQianliang added v8.0.0 Team:Code release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Sep 26, 2019

WangQianliang marked this pull request as ready for review October 10, 2019 14:04

WangQianliang requested a review from a team as a code owner October 10, 2019 14:04

WangQianliang requested review from rylnd, daveyholler and mw-ding October 10, 2019 14:06

mw-ding reviewed Oct 15, 2019

View reviewed changes

rylnd requested changes Oct 15, 2019

View reviewed changes

WangQianliang requested review from mw-ding and rylnd October 16, 2019 15:07

rylnd requested changes Oct 16, 2019

View reviewed changes

WangQianliang requested a review from rylnd October 17, 2019 13:36

daveyholler requested changes Oct 17, 2019

View reviewed changes

WangQianliang requested a review from daveyholler October 18, 2019 01:28

rylnd approved these changes Oct 18, 2019

View reviewed changes

daveyholler approved these changes Oct 18, 2019

View reviewed changes

mw-ding approved these changes Oct 18, 2019

View reviewed changes

WangQianliang added 2 commits October 19, 2019 10:18

fix(code/frontend): enable diff page

eb4b886

fix comments

e6938bc

add flag to toggle diff page

ae1c6b4

WangQianliang merged commit d5cbb47 into elastic:master Oct 21, 2019

WangQianliang deleted the code-enable-diff-page branch October 21, 2019 04:55

This was referenced Oct 21, 2019

[7.x] [Code] diff page (#46714) #48724

Merged

[7.5] [Code] diff page (#46714) #48725

Merged

WangQianliang pushed a commit that referenced this pull request Oct 21, 2019

[Code] diff page (#46714) (#48724)

6564bb9

* fix(code/frontend): enable diff page

WangQianliang pushed a commit that referenced this pull request Oct 21, 2019

[Code] diff page (#46714) (#48725)

6338d17

* fix(code/frontend): enable diff page

[Code] diff page #46714

[Code] diff page #46714

Conversation

WangQianliang commented Sep 26, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Sep 26, 2019

elasticmachine commented Sep 26, 2019

💔 Build Failed

elasticmachine commented Oct 8, 2019

💔 Build Failed

elasticmachine commented Oct 10, 2019

💔 Build Failed

elasticmachine commented Oct 10, 2019

💔 Build Failed

elasticmachine commented Oct 10, 2019

💔 Build Failed

elasticmachine commented Oct 11, 2019

💚 Build Succeeded

elasticmachine commented Oct 14, 2019

💚 Build Succeeded

elasticmachine commented Oct 14, 2019

💚 Build Succeeded

elasticmachine commented Oct 15, 2019

💚 Build Succeeded

elasticmachine commented Oct 15, 2019

💚 Build Succeeded

WangQianliang commented Oct 15, 2019

elasticmachine commented Oct 15, 2019

💚 Build Succeeded

mw-ding left a comment

Choose a reason for hiding this comment

rylnd left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 16, 2019

💚 Build Succeeded

WangQianliang commented Oct 16, 2019

elasticmachine commented Oct 16, 2019

💔 Build Failed

rylnd left a 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

elasticmachine commented Oct 17, 2019

💔 Build Failed

elasticmachine commented Oct 17, 2019

💚 Build Succeeded

daveyholler left a comment

Choose a reason for hiding this comment

rylnd commented Oct 17, 2019

WangQianliang commented Oct 18, 2019

elasticmachine commented Oct 18, 2019

💚 Build Succeeded

rylnd commented Oct 18, 2019

rylnd left a comment

Choose a reason for hiding this comment

mw-ding left a 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

elasticmachine commented Oct 19, 2019

💔 Build Failed

elasticmachine commented Oct 20, 2019

💚 Build Succeeded

elasticmachine commented Oct 21, 2019

💚 Build Succeeded

WangQianliang commented Sep 26, 2019 •

edited

Loading