From ba1bde5161acbe9c3ff354ad7dc9305dcab2da66 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Tue, 18 Dec 2018 13:46:04 +0100 Subject: [PATCH 01/17] Start writing editable diff proposal --- docs/feature-requests/005-editable-diff.md | 61 ++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 docs/feature-requests/005-editable-diff.md diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md new file mode 100644 index 0000000000..a976c58dec --- /dev/null +++ b/docs/feature-requests/005-editable-diff.md @@ -0,0 +1,61 @@ + + +**_Part 1 - Required information_** + +# Feature title + +## :tipping_hand_woman: Status + +Proposed + +## :memo: Summary + +One paragraph explanation of the feature. + +## :checkered_flag: Motivation + +Why are we doing this? What use cases does it support? What is the expected outcome? + +## 🤯 Explanation + +Explain the proposal as if it was already implemented in the GitHub package and you were describing it to an Atom user. That generally means: + +- Introducing new named concepts. +- Explaining the feature largely in terms of examples. +- Explaining any changes to existing workflows. +- Design mock-ups or diagrams depicting any new UI that will be introduced. + + +**_Part 2 - Additional information_** + +## :anchor: Drawbacks + +Why should we *not* do this? + +## :thinking: Rationale and alternatives + +- Why is this approach the best in the space of possible approaches? +- What other approaches have been considered and what is the rationale for not choosing them? +- What is the impact of not doing this? + +## :question: Unresolved questions + +- What unresolved questions do you expect to resolve through the Feature Request process before this gets merged? +- What unresolved questions do you expect to resolve through the implementation of this feature before it is released in a new version of the package? + +## :warning: Out of Scope + +- What related issues do you consider out of scope for this Feature Request that could be addressed in the future independently of the solution that comes out of this Feature Request? + +## :construction: Implementation phases + +- Can this functionality be introduced in multiple, distinct, self-contained pull requests? +- A specification for when the feature is considered "done." + +## :white_check_mark: Feature description for Atom release blog post + +- When this feature is shipped, what would we like to say or show in our Atom release blog post (example: http://blog.atom.io/2018/07/31/atom-1-29.html) +- Feel free to drop ideas and gifs here during development +- Once development is complete, write a blurb for the release coordinator to copy/paste into the Atom release blog From 9b07f125da3d558de4731da4802685059a766087 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Wed, 19 Dec 2018 10:04:46 +0100 Subject: [PATCH 02/17] brain dump atm --- docs/feature-requests/005-editable-diff.md | 52 +++++++++++++--------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index a976c58dec..123a500252 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -1,41 +1,49 @@ - - -**_Part 1 - Required information_** - -# Feature title - -## :tipping_hand_woman: Status - -Proposed +# Editable Diff ## :memo: Summary -One paragraph explanation of the feature. +Inline editing within a diff view. ## :checkered_flag: Motivation -Why are we doing this? What use cases does it support? What is the expected outcome? +- is a part of the bigger PR review workflow we want to implement +- saves user the trouble of needing to toggle to an editor and back again when you notice typos, console.log() statements, or .only() tests when reviewing unstaged changes. ## 🤯 Explanation -Explain the proposal as if it was already implemented in the GitHub package and you were describing it to an Atom user. That generally means: - -- Introducing new named concepts. -- Explaining the feature largely in terms of examples. -- Explaining any changes to existing workflows. -- Design mock-ups or diagrams depicting any new UI that will be introduced. - +- what is editable? + - staged/unstaged? + - deleted lines? + - *should be only unstaged diff and only on added lines* +- how do we indicate which diff is editable? + - VS code uses tooltip upon user trying to type something (but typing cursor thing still shows) +- at what point do we write the changes to disk? +- how much of the diff should be editable at a time? + - one hunk at a time -**_Part 2 - Additional information_** ## :anchor: Drawbacks -Why should we *not* do this? +- the diff tool in Atom is a fundamental and also old component of the package, so changing the behaviour and UI of such carries a relatively higher risk. +- no prior art to editable diffs in unified diff view (as opposed to split view discussed below.) ## :thinking: Rationale and alternatives +All of the prior arts I could find on editable diffs implement this feature with the use of "split screen diff". + +This is a gif of how it works in VS code, but other diff and/or merge tools have similar implementations: + - split screen with one side editable (the copy on disk) and the other side readonly + - both sides show unmodified lines + - readonly side shows deleted lines + - editable side shows added/modified lines + - use grey blocks to reconcile the line differences between the two sides so they line up properly + +##### Pros: + - + +##### Cons: + + - Why is this approach the best in the space of possible approaches? - What other approaches have been considered and what is the rationale for not choosing them? - What is the impact of not doing this? From a9edab9b5b382b40cadac3ec26dfd66afeb36392 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 3 Jan 2019 20:15:17 +0100 Subject: [PATCH 03/17] =?UTF-8?q?=F0=9F=92=85=20polish=20polish=20polish?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/feature-requests/005-editable-diff.md | 78 ++++++++++++++++------ 1 file changed, 58 insertions(+), 20 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index 123a500252..027306d17c 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -2,51 +2,83 @@ ## :memo: Summary -Inline editing within a diff view. +Inline editing within a diff view (i.e. `MultiFilePatchView`). ## :checkered_flag: Motivation +- saves user the trouble of needing to toggle to an editor and back again when you notice typos, `console.log()` statements, or `.only()` in tests when reviewing changes right before committing. - is a part of the bigger PR review workflow we want to implement -- saves user the trouble of needing to toggle to an editor and back again when you notice typos, console.log() statements, or .only() tests when reviewing unstaged changes. ## 🤯 Explanation -- what is editable? - - staged/unstaged? - - deleted lines? - - *should be only unstaged diff and only on added lines* -- how do we indicate which diff is editable? - - VS code uses tooltip upon user trying to type something (but typing cursor thing still shows) -- at what point do we write the changes to disk? -- how much of the diff should be editable at a time? - - one hunk at a time +The flow of using editable diff is as followed: + +#### 1. Entry point of editable state +On any given diff view, a user can get a diff hunk into an __editable state__ by: +1. double-clicking on any line within a hunk +2. clicking on a new "edit hunk" button on hunk header +3. if the hunk is selected, pressing specific keys (exact key binding TBD) +4. "edit" from context menu + +#### 2. Within an editable state +- Only one hunk is editable at a time +- Visually, it should be very clear when a hunk is in editable state. Maybe put the whole block in a box. +- The editable state behaves almost like it's a normal editor, except that the green background of added lines and red background of deleted lines will be kept. +- A deleted line will still be visible within the editable state, but it __should not be editable__. That should be communicated to user in a very clear visual manner. When navigating with keyboard, deleted lines should be skipped as if they don't exist. + +#### 3. Exiting editable state: +A user can exit the editable state by: + - clicking outside of the editable area + - pressing `esc` or `cmd-s` + - performing actions such as stage/unstage, jump to file, etc. + +Upon exiting editable state, the changes should be immediately written to the corresponding file on disk. And the diff view would re-render with the new file patch, and scroll position should remain unchanged. + +#### What is editable? + +Currently we use diff view in several places; which ones of them are editable? + - Unstaged Changes: *editable* + - Staged Changes: *editable* but with unresolved questions + - All staged changes (aka Commit Preview): *editable* but with unresolved questions + - Commit Detail Item: *NOT editable* + - Changed File Tab in PR: *editable **only** if it's a checked out PR*, also with unresolved questions ## :anchor: Drawbacks -- the diff tool in Atom is a fundamental and also old component of the package, so changing the behaviour and UI of such carries a relatively higher risk. -- no prior art to editable diffs in unified diff view (as opposed to split view discussed below.) +The diff tool in Atom is a fundamental component of the github package, so changing the behaviour and UI of such carries a relatively higher risk. + +In my research, I have found no prior art of editable diffs in *unified diff view*; in contrast, there are abundant examples of editable diffs in *split diff view* (see section below). And I think there's a reason this hasn't been done before -- making unified diff view editable could make the UI jarring and unapproachable. Some specific examples: + +- editing a previously unchanged line will result in the new file patch being longer, since we now have an newly "deleted" line. + +- editing end of a hunk might result in two hunks being joint into one (and vice versa where a hunk might get split into two). + +**The biggest UX challenge here is to elegantly transition from old file patch -> editable state -> new file patch.** + ## :thinking: Rationale and alternatives All of the prior arts I could find on editable diffs implement this feature with the use of "split screen diff". This is a gif of how it works in VS code, but other diff and/or merge tools have similar implementations: - - split screen with one side editable (the copy on disk) and the other side readonly + - split screen with one side editable (the file on disk) and the other side readonly - both sides show unmodified lines - readonly side shows deleted lines - editable side shows added/modified lines - - use grey blocks to reconcile the line differences between the two sides so they line up properly + - use grey/void blocks to reconcile the line differences between the two sides so they line up properly ##### Pros: - - + - it's easy to understand that one side is editable, and the other is not. + - adding and deleting lines are not jarring + - able to avoid the hunk separating/joining issue (mentioned above in Drawbacks section) ##### Cons: + - soft-wrap mostly doesn't work well with this view + - split view takes up a lot of screen real estate - -- Why is this approach the best in the space of possible approaches? -- What other approaches have been considered and what is the rationale for not choosing them? -- What is the impact of not doing this? +##### Rationale for not going this route: +Despite the editable split diff view being the more conventional and relatively easier approach, it diverges way too much from our existing unified diff view, and hence would not be a good direction for us. ## :question: Unresolved questions @@ -55,15 +87,21 @@ This is a gif of how it works in VS code, but other diff and/or merge tools have ## :warning: Out of Scope +#### TBD + - What related issues do you consider out of scope for this Feature Request that could be addressed in the future independently of the solution that comes out of this Feature Request? ## :construction: Implementation phases +#### TBD + - Can this functionality be introduced in multiple, distinct, self-contained pull requests? - A specification for when the feature is considered "done." ## :white_check_mark: Feature description for Atom release blog post +#### TBD + - When this feature is shipped, what would we like to say or show in our Atom release blog post (example: http://blog.atom.io/2018/07/31/atom-1-29.html) - Feel free to drop ideas and gifs here during development - Once development is complete, write a blurb for the release coordinator to copy/paste into the Atom release blog From 7bbaf1c8b4ffd103c619a963299f2379d9bee5ec Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 3 Jan 2019 20:25:17 +0100 Subject: [PATCH 04/17] :question: unresolved questions --- docs/feature-requests/005-editable-diff.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index 027306d17c..b9ca92d48f 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -6,8 +6,9 @@ Inline editing within a diff view (i.e. `MultiFilePatchView`). ## :checkered_flag: Motivation -- saves user the trouble of needing to toggle to an editor and back again when you notice typos, `console.log()` statements, or `.only()` in tests when reviewing changes right before committing. -- is a part of the bigger PR review workflow we want to implement +This can save user the trouble of needing to toggle to an editor and back again when they notice typos, `console.log()` statements, or `.only()` in tests when reviewing changes right before committing. + +This also serves as a building block of the bigger PR review workflow we want to eventually implement. ## 🤯 Explanation @@ -22,7 +23,7 @@ On any given diff view, a user can get a diff hunk into an __editable state__ by #### 2. Within an editable state - Only one hunk is editable at a time -- Visually, it should be very clear when a hunk is in editable state. Maybe put the whole block in a box. +- Visually, it should be very clear when a hunk is in editable state. Maybe put the whole block in a box? :thinking: - The editable state behaves almost like it's a normal editor, except that the green background of added lines and red background of deleted lines will be kept. - A deleted line will still be visible within the editable state, but it __should not be editable__. That should be communicated to user in a very clear visual manner. When navigating with keyboard, deleted lines should be skipped as if they don't exist. @@ -82,8 +83,9 @@ Despite the editable split diff view being the more conventional and relatively ## :question: Unresolved questions -- What unresolved questions do you expect to resolve through the Feature Request process before this gets merged? -- What unresolved questions do you expect to resolve through the implementation of this feature before it is released in a new version of the package? +- Currently, if a user make changed to a staged file, the new changes show up in Unstaged Changes, but are not applied to the already staged file. If we allow staged file to be edited, should the new changes apply to both file on disk as well as the staged entry? + +- The PR comments we are implementing in #1856 already add consierable complexity to the diff view. Should the comments be visible when the diff is in editable state? ## :warning: Out of Scope From 9e3872e34ef12ca664bc75543fba854b402fbbb7 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 3 Jan 2019 20:32:33 +0100 Subject: [PATCH 05/17] add screenshots in drawback section --- docs/feature-requests/005-editable-diff.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index b9ca92d48f..4903b3d6af 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -53,8 +53,16 @@ In my research, I have found no prior art of editable diffs in *unified diff vie - editing a previously unchanged line will result in the new file patch being longer, since we now have an newly "deleted" line. +| before | after | +| --- | --- | +|screen shot 2019-01-03 at 19 04 58|screen shot 2019-01-03 at 19 05 41| + - editing end of a hunk might result in two hunks being joint into one (and vice versa where a hunk might get split into two). +| before | after | +| --- | --- | +|screen shot 2019-01-03 at 19 19 32|screen shot 2019-01-03 at 19 20 23| + **The biggest UX challenge here is to elegantly transition from old file patch -> editable state -> new file patch.** From a62a1c81bdb4e7b5f2194bf8dd194a1e6d1370be Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 3 Jan 2019 20:37:47 +0100 Subject: [PATCH 06/17] add vscode screenshot --- docs/feature-requests/005-editable-diff.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index 4903b3d6af..565723e650 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -70,6 +70,7 @@ In my research, I have found no prior art of editable diffs in *unified diff vie All of the prior arts I could find on editable diffs implement this feature with the use of "split screen diff". +![kapture 2019-01-03 at 20 35 29](https://user-images.githubusercontent.com/6842965/50657589-37134980-0f97-11e9-96cc-41cb1eda6546.gif) This is a gif of how it works in VS code, but other diff and/or merge tools have similar implementations: - split screen with one side editable (the file on disk) and the other side readonly - both sides show unmodified lines From 1e27a6c945282a8795f378c46d09fe9b618383c7 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 3 Jan 2019 20:44:43 +0100 Subject: [PATCH 07/17] add some links --- docs/feature-requests/005-editable-diff.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index 565723e650..c6c1c9e9bb 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -8,7 +8,7 @@ Inline editing within a diff view (i.e. `MultiFilePatchView`). This can save user the trouble of needing to toggle to an editor and back again when they notice typos, `console.log()` statements, or `.only()` in tests when reviewing changes right before committing. -This also serves as a building block of the bigger PR review workflow we want to eventually implement. +This also serves as a building block of the bigger [PR review workflow](https://github.com/atom/github/blob/master/docs/feature-requests/003-pull-request-review.md) we want to eventually implement. ## 🤯 Explanation @@ -39,32 +39,32 @@ Upon exiting editable state, the changes should be immediately written to the co Currently we use diff view in several places; which ones of them are editable? - Unstaged Changes: *editable* - - Staged Changes: *editable* but with unresolved questions - - All staged changes (aka Commit Preview): *editable* but with unresolved questions + - Staged Changes: *editable* but with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) + - All staged changes (aka Commit Preview): *editable* but with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) - Commit Detail Item: *NOT editable* - - Changed File Tab in PR: *editable **only** if it's a checked out PR*, also with unresolved questions + - Changed File Tab in PR: *editable **only** if it's a checked out PR*, also with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) ## :anchor: Drawbacks The diff tool in Atom is a fundamental component of the github package, so changing the behaviour and UI of such carries a relatively higher risk. -In my research, I have found no prior art of editable diffs in *unified diff view*; in contrast, there are abundant examples of editable diffs in *split diff view* (see section below). And I think there's a reason this hasn't been done before -- making unified diff view editable could make the UI jarring and unapproachable. Some specific examples: +In my research, I have found no prior art of editable diffs in *unified diff view*; in contrast, there are abundant examples of editable diffs in *split diff view* (see section below). And I think there's a reason this hasn't been done before -- making unified diff view editable could make the UI jarring and unapproachable, since there are considerable visual differences between old (before editing) file patch and new (after editing) file patch, and the editable state sits somewhere between the two. **The biggest UX challenge here is to elegantly transition from old file patch -> editable state -> new file patch.** + +Some specific examples: - editing a previously unchanged line will result in the new file patch being longer, since we now have an newly "deleted" line. -| before | after | +| old file patch | new file patch | | --- | --- | |screen shot 2019-01-03 at 19 04 58|screen shot 2019-01-03 at 19 05 41| - editing end of a hunk might result in two hunks being joint into one (and vice versa where a hunk might get split into two). -| before | after | +| old file patch | new file patch | | --- | --- | |screen shot 2019-01-03 at 19 19 32|screen shot 2019-01-03 at 19 20 23| -**The biggest UX challenge here is to elegantly transition from old file patch -> editable state -> new file patch.** - ## :thinking: Rationale and alternatives @@ -94,7 +94,7 @@ Despite the editable split diff view being the more conventional and relatively - Currently, if a user make changed to a staged file, the new changes show up in Unstaged Changes, but are not applied to the already staged file. If we allow staged file to be edited, should the new changes apply to both file on disk as well as the staged entry? -- The PR comments we are implementing in #1856 already add consierable complexity to the diff view. Should the comments be visible when the diff is in editable state? +- The PR comments we are implementing in [#1856](https://github.com/atom/github/pull/1856) already add consierable complexity to the diff view. Should the comments be visible when the diff is in editable state? ## :warning: Out of Scope From 2621bc809608165c4b7c1fc5e7eb8db8b6ef82ed Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Fri, 4 Jan 2019 13:11:06 +0100 Subject: [PATCH 08/17] change up wording --- docs/feature-requests/005-editable-diff.md | 28 +++++++++++++--------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index c6c1c9e9bb..fa44567885 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -16,24 +16,30 @@ The flow of using editable diff is as followed: #### 1. Entry point of editable state On any given diff view, a user can get a diff hunk into an __editable state__ by: -1. double-clicking on any line within a hunk -2. clicking on a new "edit hunk" button on hunk header -3. if the hunk is selected, pressing specific keys (exact key binding TBD) -4. "edit" from context menu + - double-clicking on any line within a hunk + - clicking on a new "edit hunk" button on hunk header + - if the hunk is selected, pressing specific keys (exact key binding TBD) + - "edit" from context menu + - if already in editable state in a previous hunk, navigating with keyboard can exit previous hunk's editable state and enter editable state of the next hunk. + #### 2. Within an editable state -- Only one hunk is editable at a time -- Visually, it should be very clear when a hunk is in editable state. Maybe put the whole block in a box? :thinking: -- The editable state behaves almost like it's a normal editor, except that the green background of added lines and red background of deleted lines will be kept. -- A deleted line will still be visible within the editable state, but it __should not be editable__. That should be communicated to user in a very clear visual manner. When navigating with keyboard, deleted lines should be skipped as if they don't exist. + - Only one hunk is editable at a time + - Visually, it should be very clear when a hunk is in editable state. In addition to a blinking caret, maybe we can put the whole block in a box or fade out the rest of the diff? :thinking: + - The editable state behaves almost like it's a normal editor, except that the green background of added lines and red background of deleted lines will be kept. + - A deleted line will still be visible within the editable state, but it _is not editable_. That should be communicated to user in a very clear visual manner: + - The cursor for hovering over deleted lines should show up as arrow as opposed to text cursor. + - When navigating with keyboard, deleted lines should be skipped as if they don't exist. + - When dragging cursor to highlight a block of text, deleted lines should not get highlighted. #### 3. Exiting editable state: +Upon exiting editable state, the changes should be immediately saved to the corresponding file on disk. And the diff view would re-render with the new file patch, and scroll position should remain unchanged. + A user can exit the editable state by: - clicking outside of the editable area - pressing `esc` or `cmd-s` - performing actions such as stage/unstage, jump to file, etc. -Upon exiting editable state, the changes should be immediately written to the corresponding file on disk. And the diff view would re-render with the new file patch, and scroll position should remain unchanged. #### What is editable? @@ -47,9 +53,9 @@ Currently we use diff view in several places; which ones of them are editable? ## :anchor: Drawbacks -The diff tool in Atom is a fundamental component of the github package, so changing the behaviour and UI of such carries a relatively higher risk. +The diff tool in Atom is a fundamental component of the GitHub package, so changing the behaviour and UI of such carries a relatively higher risk. -In my research, I have found no prior art of editable diffs in *unified diff view*; in contrast, there are abundant examples of editable diffs in *split diff view* (see section below). And I think there's a reason this hasn't been done before -- making unified diff view editable could make the UI jarring and unapproachable, since there are considerable visual differences between old (before editing) file patch and new (after editing) file patch, and the editable state sits somewhere between the two. **The biggest UX challenge here is to elegantly transition from old file patch -> editable state -> new file patch.** +In my research, I have found no prior art of editable diffs in *unified diff view*; in contrast, there are abundant examples of editable diffs in *split diff view* (see section below). And I think there's a reason this hasn't been done before -- making unified diff view editable could make the UI jarring and unapproachable, since there are considerable visual differences between old (before editing) file patch and new (after editing) file patch, and the editable state sits somewhere between the two. **The challenge here is to elegantly transition from old file patch -> editable state -> new file patch.** Some specific examples: From 5653d6d08d9bd9a5a92dfdfa64e9dea96a6ee9d1 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Fri, 4 Jan 2019 14:24:12 +0100 Subject: [PATCH 09/17] new proposed flow based on design chat with @simurai Co-Authored-By: simurai --- docs/feature-requests/005-editable-diff.md | 85 ++++++++++++++++------ 1 file changed, 63 insertions(+), 22 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index fa44567885..d9e84cdbb9 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -12,33 +12,32 @@ This also serves as a building block of the bigger [PR review workflow](https:// ## 🤯 Explanation -The flow of using editable diff is as followed: - #### 1. Entry point of editable state -On any given diff view, a user can get a diff hunk into an __editable state__ by: - - double-clicking on any line within a hunk - - clicking on a new "edit hunk" button on hunk header - - if the hunk is selected, pressing specific keys (exact key binding TBD) - - "edit" from context menu - - if already in editable state in a previous hunk, navigating with keyboard can exit previous hunk's editable state and enter editable state of the next hunk. + +- By default, a diff view is editable (with some [exceptions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#what-is-editable)). By clicking into any diff, a blinking caret will appear to signify users they can start editing. +- If a diff is readonly, there will be a visual indicator of such: +editable vs readonly diff #### 2. Within an editable state - - Only one hunk is editable at a time - - Visually, it should be very clear when a hunk is in editable state. In addition to a blinking caret, maybe we can put the whole block in a box or fade out the rest of the diff? :thinking: - - The editable state behaves almost like it's a normal editor, except that the green background of added lines and red background of deleted lines will be kept. - - A deleted line will still be visible within the editable state, but it _is not editable_. That should be communicated to user in a very clear visual manner: - - The cursor for hovering over deleted lines should show up as arrow as opposed to text cursor. - - When navigating with keyboard, deleted lines should be skipped as if they don't exist. - - When dragging cursor to highlight a block of text, deleted lines should not get highlighted. +- The editable state behaves almost like a normal editor, except that the green background of added lines and red background of deleted lines will be kept. +- Behaviours such as stage/unstage hunk and selected line will remain the same, since the line being actively edited will still be considered as "selected", hence show up as highlighted. +- A deleted line will still be visible within the editable state, but it _is not editable_. That will be communicated to user in a very clear visual manner: + - The cursor for hovering over deleted lines will show up as arrow as opposed to text cursor. + - Deleted lines can still be selected (via mouse or keyboard) but no caret will be visible; basically same as current behaviour. -#### 3. Exiting editable state: -Upon exiting editable state, the changes should be immediately saved to the corresponding file on disk. And the diff view would re-render with the new file patch, and scroll position should remain unchanged. -A user can exit the editable state by: - - clicking outside of the editable area - - pressing `esc` or `cmd-s` - - performing actions such as stage/unstage, jump to file, etc. +#### 3. Save (i.e. write modification to disk) + +- As a user make edits in a diff view, the file name will have some visual cue indicating there's unsaved modification, akin to when a regular atom editor has unsaved changes. + - For multiple file diff views, the unsaved modification indicator will be on a per-file basis. In the following example, `src/fish.txt` has unsaved modification but `something` does not: + unsaved modification indicator + + +- No edits within the diff view will be written to disk until user explicitly saves (`cmd-s`) the modified diff. + - For multple file diff view, one "save" action will save _all modifications across the different files within that diff view_. + +- Upon saving, the diff view will re-render with the new file patch, and scroll position will remain unchanged. Some visual "jumps" might result as the new file patch is shown (see [examples](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#some-specific-examples)), but that won't be too jarring since the save action is so explicit. #### What is editable? @@ -57,7 +56,7 @@ The diff tool in Atom is a fundamental component of the GitHub package, so chang In my research, I have found no prior art of editable diffs in *unified diff view*; in contrast, there are abundant examples of editable diffs in *split diff view* (see section below). And I think there's a reason this hasn't been done before -- making unified diff view editable could make the UI jarring and unapproachable, since there are considerable visual differences between old (before editing) file patch and new (after editing) file patch, and the editable state sits somewhere between the two. **The challenge here is to elegantly transition from old file patch -> editable state -> new file patch.** -Some specific examples: +##### Some specific examples: - editing a previously unchanged line will result in the new file patch being longer, since we now have an newly "deleted" line. @@ -74,6 +73,9 @@ Some specific examples: ## :thinking: Rationale and alternatives + +### Editable Split Diff + All of the prior arts I could find on editable diffs implement this feature with the use of "split screen diff". ![kapture 2019-01-03 at 20 35 29](https://user-images.githubusercontent.com/6842965/50657589-37134980-0f97-11e9-96cc-41cb1eda6546.gif) @@ -96,12 +98,51 @@ This is a gif of how it works in VS code, but other diff and/or merge tools have ##### Rationale for not going this route: Despite the editable split diff view being the more conventional and relatively easier approach, it diverges way too much from our existing unified diff view, and hence would not be a good direction for us. +### First iteration + +
+ +iteration 1 (by default, a diff view is not editable) + +The flow of using editable diff is as followed: + +#### 1. Entry point of editable state +On any given diff view, a user can get a diff hunk into an __editable state__ by: + - double-clicking on any line within a hunk + - clicking on a new "edit hunk" button on hunk header + - if the hunk is selected, pressing specific keys (exact key binding TBD) + - "edit" from context menu + - if already in editable state in a previous hunk, navigating with keyboard can exit previous hunk's editable state and enter editable state of the next hunk. + + +#### 2. Within an editable state + - Only one hunk is editable at a time + - Visually, it will be very clear when a hunk is in editable state. In addition to a blinking caret, maybe we can put the whole block in a box or fade out the rest of the diff? :thinking: + - The editable state behaves almost like it's a normal editor, except that the green background of added lines and red background of deleted lines will be kept. + - A deleted line will still be visible within the editable state, but it _is not editable_. That will be communicated to user in a very clear visual manner: + - The cursor for hovering over deleted lines will show up as arrow as opposed to text cursor. + - When navigating with keyboard, deleted lines will be skipped as if they don't exist. + - When dragging cursor to highlight a block of text, deleted lines will not get highlighted. + +#### 3. Exiting editable state: +Upon exiting editable state, the changes will be immediately saved to the corresponding file on disk. And the diff view would re-render with the new file patch, and scroll position will remain unchanged. + +A user can exit the editable state by: + - clicking outside of the editable area + - pressing `esc` or `cmd-s` + - performing actions such as stage/unstage, jump to file, etc. + +
+ + ## :question: Unresolved questions - Currently, if a user make changed to a staged file, the new changes show up in Unstaged Changes, but are not applied to the already staged file. If we allow staged file to be edited, should the new changes apply to both file on disk as well as the staged entry? - The PR comments we are implementing in [#1856](https://github.com/atom/github/pull/1856) already add consierable complexity to the diff view. Should the comments be visible when the diff is in editable state? +- When modifying a diff, should actions such as stage/unstage, jump to file, etc. trigger a "save"? + ## :warning: Out of Scope #### TBD From 450ca9e238ea932d2d07320f66201d94f1654267 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Fri, 4 Jan 2019 14:49:01 +0100 Subject: [PATCH 10/17] include rationale for not going with iteration 1 --- docs/feature-requests/005-editable-diff.md | 71 ++++++++++++---------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index d9e84cdbb9..c26c763884 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -25,19 +25,19 @@ This also serves as a building block of the bigger [PR review workflow](https:// - A deleted line will still be visible within the editable state, but it _is not editable_. That will be communicated to user in a very clear visual manner: - The cursor for hovering over deleted lines will show up as arrow as opposed to text cursor. - Deleted lines can still be selected (via mouse or keyboard) but no caret will be visible; basically same as current behaviour. +- As a user make edits in a diff view, the file name will have some visual cue indicating there's unsaved modification, akin to when a regular atom editor has unsaved changes. +- For multiple file diff views, the unsaved modification indicator will be on a per-file basis. In the following example, `src/fish.txt` has unsaved modification but `something` does not: +unsaved modification indicator +- When attempting to close a diff with unsaved modification, user will be prompted with "are you sure", etc -- same behaviour as an unsaved Atom editor #### 3. Save (i.e. write modification to disk) -- As a user make edits in a diff view, the file name will have some visual cue indicating there's unsaved modification, akin to when a regular atom editor has unsaved changes. - - For multiple file diff views, the unsaved modification indicator will be on a per-file basis. In the following example, `src/fish.txt` has unsaved modification but `something` does not: - unsaved modification indicator - - - No edits within the diff view will be written to disk until user explicitly saves (`cmd-s`) the modified diff. - For multple file diff view, one "save" action will save _all modifications across the different files within that diff view_. -- Upon saving, the diff view will re-render with the new file patch, and scroll position will remain unchanged. Some visual "jumps" might result as the new file patch is shown (see [examples](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#some-specific-examples)), but that won't be too jarring since the save action is so explicit. +- Upon saving, the diff view will re-render with the new file patch, and scroll position will remain unchanged. +- Some visual jumps might result as the new file patch is shown (see [examples](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#some-specific-examples)), but that won't be too jarring since the save action is so explicit. #### What is editable? @@ -73,36 +73,13 @@ In my research, I have found no prior art of editable diffs in *unified diff vie ## :thinking: Rationale and alternatives +### Default to read-only state -### Editable Split Diff - -All of the prior arts I could find on editable diffs implement this feature with the use of "split screen diff". - -![kapture 2019-01-03 at 20 35 29](https://user-images.githubusercontent.com/6842965/50657589-37134980-0f97-11e9-96cc-41cb1eda6546.gif) -This is a gif of how it works in VS code, but other diff and/or merge tools have similar implementations: - - split screen with one side editable (the file on disk) and the other side readonly - - both sides show unmodified lines - - readonly side shows deleted lines - - editable side shows added/modified lines - - use grey/void blocks to reconcile the line differences between the two sides so they line up properly - -##### Pros: - - it's easy to understand that one side is editable, and the other is not. - - adding and deleting lines are not jarring - - able to avoid the hunk separating/joining issue (mentioned above in Drawbacks section) - -##### Cons: - - soft-wrap mostly doesn't work well with this view - - split view takes up a lot of screen real estate - -##### Rationale for not going this route: -Despite the editable split diff view being the more conventional and relatively easier approach, it diverges way too much from our existing unified diff view, and hence would not be a good direction for us. - -### First iteration +A first iteration was considered where by default, a diff view is read-only, and users would have to explicitly toggle in and out of an editable state.
-iteration 1 (by default, a diff view is not editable) +More details of the workflow considered in iteration 1 The flow of using editable diff is as followed: @@ -134,6 +111,36 @@ A user can exit the editable state by:
+##### Rationale for not going this route: +- Having to explicitly toggle into an editable state is similar to behaviour seen in dotcom, but this goes against our editor-first approach. It's called an _editor_ for a reason afterall -- things are expected to be editable. +- Since [the majority of the existing diff view usage will be editable](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#what-is-editable), it makes sense to default to the editable state, and only communicate to users when it's an exception (i.e. read-only). + + +### Editable Split Diff + +All of the prior arts I could find on editable diffs implement this feature with the use of "split screen diff". + +![kapture 2019-01-03 at 20 35 29](https://user-images.githubusercontent.com/6842965/50657589-37134980-0f97-11e9-96cc-41cb1eda6546.gif) + +This is a gif of how it works in VS code, but other diff and/or merge tools have similar implementations: + - split screen with one side editable (the file on disk) and the other side readonly + - both sides show unmodified lines + - readonly side shows deleted lines + - editable side shows added/modified lines + - use grey/void blocks to reconcile the line differences between the two sides so they line up properly + +##### Pros: + - it's easy to understand that one side is editable, and the other is not. + - adding and deleting lines are not jarring + - able to avoid the hunk separating/joining issue (mentioned above in Drawbacks section) + +##### Cons: + - soft-wrap mostly doesn't work well with this view + - split view takes up a lot of screen real estate + +##### Rationale for not going this route: +Despite the editable split diff view being the more conventional and relatively easier approach, it diverges way too much from our existing unified diff view, and hence would not be a good direction for us. + ## :question: Unresolved questions From ee8c43d731c0943647d24b451e530f54ac36e358 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Fri, 4 Jan 2019 14:49:18 +0100 Subject: [PATCH 11/17] move `what is editable` to the top --- docs/feature-requests/005-editable-diff.md | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index c26c763884..b4fb26fc7e 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -12,6 +12,15 @@ This also serves as a building block of the bigger [PR review workflow](https:// ## 🤯 Explanation +#### What is editable? + +Currently we use diff view in several places; which ones of them are editable? + - Unstaged Changes: *editable* + - Staged Changes: *editable* but with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) + - All staged changes (aka Commit Preview): *editable* but with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) + - Commit Detail Item: *NOT editable* + - Changed File Tab in PR: *editable **only** if it's a checked out PR*, also with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) + #### 1. Entry point of editable state - By default, a diff view is editable (with some [exceptions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#what-is-editable)). By clicking into any diff, a blinking caret will appear to signify users they can start editing. @@ -40,16 +49,6 @@ This also serves as a building block of the bigger [PR review workflow](https:// - Some visual jumps might result as the new file patch is shown (see [examples](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#some-specific-examples)), but that won't be too jarring since the save action is so explicit. -#### What is editable? - -Currently we use diff view in several places; which ones of them are editable? - - Unstaged Changes: *editable* - - Staged Changes: *editable* but with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) - - All staged changes (aka Commit Preview): *editable* but with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) - - Commit Detail Item: *NOT editable* - - Changed File Tab in PR: *editable **only** if it's a checked out PR*, also with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) - - ## :anchor: Drawbacks The diff tool in Atom is a fundamental component of the GitHub package, so changing the behaviour and UI of such carries a relatively higher risk. From 6dff853ea2b7f17e35f4f178023eb4605c6dd851 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Fri, 4 Jan 2019 17:28:25 +0100 Subject: [PATCH 12/17] fix some links --- docs/feature-requests/005-editable-diff.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index b4fb26fc7e..a5b45d6615 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -16,14 +16,14 @@ This also serves as a building block of the bigger [PR review workflow](https:// Currently we use diff view in several places; which ones of them are editable? - Unstaged Changes: *editable* - - Staged Changes: *editable* but with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) - - All staged changes (aka Commit Preview): *editable* but with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) + - Staged Changes: *editable* but with [unresolved questions](#question-unresolved-questions) + - All staged changes (aka Commit Preview): *editable* but with [unresolved questions](#question-unresolved-questions) - Commit Detail Item: *NOT editable* - - Changed File Tab in PR: *editable **only** if it's a checked out PR*, also with [unresolved questions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#question-unresolved-questions) + - Changed File Tab in PR: *editable **only** if it's a checked out PR*, also with [unresolved questions](#question-unresolved-questions) #### 1. Entry point of editable state -- By default, a diff view is editable (with some [exceptions](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#what-is-editable)). By clicking into any diff, a blinking caret will appear to signify users they can start editing. +- By default, a diff view is editable (with some [exceptions](#what-is-editable) listed above). By clicking into any diff, a blinking caret will appear to signify users they can start editing. - If a diff is readonly, there will be a visual indicator of such: editable vs readonly diff @@ -46,7 +46,7 @@ Currently we use diff view in several places; which ones of them are editable? - For multple file diff view, one "save" action will save _all modifications across the different files within that diff view_. - Upon saving, the diff view will re-render with the new file patch, and scroll position will remain unchanged. -- Some visual jumps might result as the new file patch is shown (see [examples](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#some-specific-examples)), but that won't be too jarring since the save action is so explicit. +- Some visual jumps might result as the new file patch is shown (see [examples](#some-specific-examples)), but that won't be too jarring since the save action is so explicit. ## :anchor: Drawbacks @@ -112,7 +112,7 @@ A user can exit the editable state by: ##### Rationale for not going this route: - Having to explicitly toggle into an editable state is similar to behaviour seen in dotcom, but this goes against our editor-first approach. It's called an _editor_ for a reason afterall -- things are expected to be editable. -- Since [the majority of the existing diff view usage will be editable](https://github.com/atom/github/blob/vy/proposal-editable-diff/docs/feature-requests/005-editable-diff.md#what-is-editable), it makes sense to default to the editable state, and only communicate to users when it's an exception (i.e. read-only). +- Since [the majority of the existing diff view usage will be editable](#what-is-editable), it makes sense to default to the editable state, and only communicate to users when it's an exception (i.e. read-only). ### Editable Split Diff From f86ddd126afcf44273817b7cac666db2461a33cb Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Fri, 4 Jan 2019 17:39:41 +0100 Subject: [PATCH 13/17] grammar and typo --- docs/feature-requests/005-editable-diff.md | 60 +++++++++++----------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index a5b45d6615..5577ad803c 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -6,44 +6,46 @@ Inline editing within a diff view (i.e. `MultiFilePatchView`). ## :checkered_flag: Motivation -This can save user the trouble of needing to toggle to an editor and back again when they notice typos, `console.log()` statements, or `.only()` in tests when reviewing changes right before committing. +This can save users the trouble of needing to toggle to an editor and back again when they notice typos, `console.log()` statements, or `.only()` in tests when reviewing changes right before committing. This also serves as a building block of the bigger [PR review workflow](https://github.com/atom/github/blob/master/docs/feature-requests/003-pull-request-review.md) we want to eventually implement. ## 🤯 Explanation -#### What is editable? +### What is editable? -Currently we use diff view in several places; which ones of them are editable? +Currently, we use diff view in several places; which ones of them are editable? - Unstaged Changes: *editable* - Staged Changes: *editable* but with [unresolved questions](#question-unresolved-questions) - All staged changes (aka Commit Preview): *editable* but with [unresolved questions](#question-unresolved-questions) - Commit Detail Item: *NOT editable* - Changed File Tab in PR: *editable **only** if it's a checked out PR*, also with [unresolved questions](#question-unresolved-questions) -#### 1. Entry point of editable state +### Workflow of editing a diff + +#### 1. The entry point of editable state - By default, a diff view is editable (with some [exceptions](#what-is-editable) listed above). By clicking into any diff, a blinking caret will appear to signify users they can start editing. -- If a diff is readonly, there will be a visual indicator of such: -editable vs readonly diff +- If a diff is read-only, there will be a visual indicator of such: +editable vs read-only diff #### 2. Within an editable state - The editable state behaves almost like a normal editor, except that the green background of added lines and red background of deleted lines will be kept. -- Behaviours such as stage/unstage hunk and selected line will remain the same, since the line being actively edited will still be considered as "selected", hence show up as highlighted. -- A deleted line will still be visible within the editable state, but it _is not editable_. That will be communicated to user in a very clear visual manner: - - The cursor for hovering over deleted lines will show up as arrow as opposed to text cursor. +- Behaviours such as stage/unstage hunk and selected line will remain the same, since the line that is being actively edited will still be considered as "selected", hence show up as highlighted. +- A deleted line will still be visible within the editable state, but it _is not editable_. That will be communicated to users in a very clear visual manner: + - The cursor for hovering over deleted lines will show up as an arrow as opposed to a text cursor. - Deleted lines can still be selected (via mouse or keyboard) but no caret will be visible; basically same as current behaviour. -- As a user make edits in a diff view, the file name will have some visual cue indicating there's unsaved modification, akin to when a regular atom editor has unsaved changes. -- For multiple file diff views, the unsaved modification indicator will be on a per-file basis. In the following example, `src/fish.txt` has unsaved modification but `something` does not: +- As a user makes edits in a diff view, the file name will have some visual cue indicating there are unsaved modifications, akin to when a regular atom editor has unsaved changes. + - For multiple file diff views, the unsaved modification indicator will be on a per-file basis. In the following example, `src/fish.txt` has unsaved modification but `something` does not: unsaved modification indicator -- When attempting to close a diff with unsaved modification, user will be prompted with "are you sure", etc -- same behaviour as an unsaved Atom editor +- When attempting to close a diff with unsaved modification, users will be prompted with "are you sure", etc -- same behaviour as an unsaved Atom editor #### 3. Save (i.e. write modification to disk) -- No edits within the diff view will be written to disk until user explicitly saves (`cmd-s`) the modified diff. - - For multple file diff view, one "save" action will save _all modifications across the different files within that diff view_. +- No edits within the diff view will be written to disk until the user explicitly saves (`cmd-s`) the modified diff. + - For multiple file diff view, one "save" action will save _all modifications across the different files within that diff view_. - Upon saving, the diff view will re-render with the new file patch, and scroll position will remain unchanged. - Some visual jumps might result as the new file patch is shown (see [examples](#some-specific-examples)), but that won't be too jarring since the save action is so explicit. @@ -57,7 +59,7 @@ In my research, I have found no prior art of editable diffs in *unified diff vie ##### Some specific examples: -- editing a previously unchanged line will result in the new file patch being longer, since we now have an newly "deleted" line. +- editing a previously unchanged line will result in the new file patch being longer since we now have a newly "deleted" line. | old file patch | new file patch | | --- | --- | @@ -82,36 +84,36 @@ A first iteration was considered where by default, a diff view is read-only, and The flow of using editable diff is as followed: -#### 1. Entry point of editable state +#### 1. The entry point of editable state On any given diff view, a user can get a diff hunk into an __editable state__ by: - double-clicking on any line within a hunk - clicking on a new "edit hunk" button on hunk header - if the hunk is selected, pressing specific keys (exact key binding TBD) - - "edit" from context menu - - if already in editable state in a previous hunk, navigating with keyboard can exit previous hunk's editable state and enter editable state of the next hunk. + - "edit" from the context menu + - if already in the editable state in a previous hunk, navigating with the keyboard can exit previous hunk's editable state and enter the editable state of the next hunk. #### 2. Within an editable state - Only one hunk is editable at a time - - Visually, it will be very clear when a hunk is in editable state. In addition to a blinking caret, maybe we can put the whole block in a box or fade out the rest of the diff? :thinking: + - Visually, it will be very clear when a hunk is in the editable state. In addition to a blinking caret, maybe we can put the whole block in a box or fade out the rest of the diff? :thinking: - The editable state behaves almost like it's a normal editor, except that the green background of added lines and red background of deleted lines will be kept. - - A deleted line will still be visible within the editable state, but it _is not editable_. That will be communicated to user in a very clear visual manner: - - The cursor for hovering over deleted lines will show up as arrow as opposed to text cursor. - - When navigating with keyboard, deleted lines will be skipped as if they don't exist. - - When dragging cursor to highlight a block of text, deleted lines will not get highlighted. + - A deleted line will still be visible within the editable state, but it _is not editable_. That will be communicated to users in a very clear visual manner: + - The cursor for hovering over deleted lines will show up as an arrow as opposed to text cursor. + - When navigating with the keyboard, deleted lines will be skipped as if they don't exist. + - When dragging the cursor to highlight a block of text, deleted lines will not get highlighted. #### 3. Exiting editable state: Upon exiting editable state, the changes will be immediately saved to the corresponding file on disk. And the diff view would re-render with the new file patch, and scroll position will remain unchanged. A user can exit the editable state by: - clicking outside of the editable area - - pressing `esc` or `cmd-s` + - pressing `ESC` or `cmd-s` - performing actions such as stage/unstage, jump to file, etc. ##### Rationale for not going this route: -- Having to explicitly toggle into an editable state is similar to behaviour seen in dotcom, but this goes against our editor-first approach. It's called an _editor_ for a reason afterall -- things are expected to be editable. +- Having to explicitly toggle into an editable state is similar to behaviour seen in dotcom, but this goes against our editor-first approach. It's called an _editor_ for a reason after all -- things are expected to be editable. - Since [the majority of the existing diff view usage will be editable](#what-is-editable), it makes sense to default to the editable state, and only communicate to users when it's an exception (i.e. read-only). @@ -122,7 +124,7 @@ All of the prior arts I could find on editable diffs implement this feature with ![kapture 2019-01-03 at 20 35 29](https://user-images.githubusercontent.com/6842965/50657589-37134980-0f97-11e9-96cc-41cb1eda6546.gif) This is a gif of how it works in VS code, but other diff and/or merge tools have similar implementations: - - split screen with one side editable (the file on disk) and the other side readonly + - split screen with one side editable (the file on disk) and the other side read-only - both sides show unmodified lines - readonly side shows deleted lines - editable side shows added/modified lines @@ -135,7 +137,7 @@ This is a gif of how it works in VS code, but other diff and/or merge tools have ##### Cons: - soft-wrap mostly doesn't work well with this view - - split view takes up a lot of screen real estate + - the split view takes up a lot of screen real estate ##### Rationale for not going this route: Despite the editable split diff view being the more conventional and relatively easier approach, it diverges way too much from our existing unified diff view, and hence would not be a good direction for us. @@ -143,9 +145,9 @@ Despite the editable split diff view being the more conventional and relatively ## :question: Unresolved questions -- Currently, if a user make changed to a staged file, the new changes show up in Unstaged Changes, but are not applied to the already staged file. If we allow staged file to be edited, should the new changes apply to both file on disk as well as the staged entry? +- Currently, if a user makes changed to a staged file, the new changes show up in Unstaged Changes, but are not applied to the already staged file. If we allow a staged file to be edited, should the new changes apply to both file on disk as well as the staged entry? -- The PR comments we are implementing in [#1856](https://github.com/atom/github/pull/1856) already add consierable complexity to the diff view. Should the comments be visible when the diff is in editable state? +- The PR comments we are implementing in [#1856](https://github.com/atom/github/pull/1856) already add considerable complexity to the diff view. Should the comments be visible when the diff is in an editable state? - When modifying a diff, should actions such as stage/unstage, jump to file, etc. trigger a "save"? From fe76c19fcdc26db022b6cd4fb12e9cf1ce16e7ea Mon Sep 17 00:00:00 2001 From: simurai Date: Mon, 7 Jan 2019 18:09:43 +0900 Subject: [PATCH 14/17] Update editable vs read-only diffs --- docs/feature-requests/005-editable-diff.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index 5577ad803c..545b84b3f6 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -26,8 +26,9 @@ Currently, we use diff view in several places; which ones of them are editable? #### 1. The entry point of editable state - By default, a diff view is editable (with some [exceptions](#what-is-editable) listed above). By clicking into any diff, a blinking caret will appear to signify users they can start editing. -- If a diff is read-only, there will be a visual indicator of such: -editable vs read-only diff + ![editable](https://user-images.githubusercontent.com/378023/50758847-b27e3f00-12a6-11e9-8c2e-6c0bf085fdbd.gif) +- If a diff is read-only, there will be no caret. The mouse cursor shows as "default" arrow. Selecting still works. + ![non-editable](https://user-images.githubusercontent.com/378023/50758848-b27e3f00-12a6-11e9-996c-8a458196e070.gif) #### 2. Within an editable state From f3ed368387c925154fca63c7d7679a787e2f2c52 Mon Sep 17 00:00:00 2001 From: simurai Date: Mon, 7 Jan 2019 18:27:09 +0900 Subject: [PATCH 15/17] Add gif for deleted vs added lines --- docs/feature-requests/005-editable-diff.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index 545b84b3f6..0651cd53eb 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -37,6 +37,7 @@ Currently, we use diff view in several places; which ones of them are editable? - A deleted line will still be visible within the editable state, but it _is not editable_. That will be communicated to users in a very clear visual manner: - The cursor for hovering over deleted lines will show up as an arrow as opposed to a text cursor. - Deleted lines can still be selected (via mouse or keyboard) but no caret will be visible; basically same as current behaviour. + ![deleted](https://user-images.githubusercontent.com/378023/50759911-b3fd3680-12a9-11e9-8332-abf211e0743c.gif) - As a user makes edits in a diff view, the file name will have some visual cue indicating there are unsaved modifications, akin to when a regular atom editor has unsaved changes. - For multiple file diff views, the unsaved modification indicator will be on a per-file basis. In the following example, `src/fish.txt` has unsaved modification but `something` does not: unsaved modification indicator From 740b205728a6030f00c775bebd08c79812a14578 Mon Sep 17 00:00:00 2001 From: simurai Date: Wed, 9 Jan 2019 17:07:39 +0900 Subject: [PATCH 16/17] Update gifs --- docs/feature-requests/005-editable-diff.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index 0651cd53eb..f80d6c5d55 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -26,10 +26,9 @@ Currently, we use diff view in several places; which ones of them are editable? #### 1. The entry point of editable state - By default, a diff view is editable (with some [exceptions](#what-is-editable) listed above). By clicking into any diff, a blinking caret will appear to signify users they can start editing. - ![editable](https://user-images.githubusercontent.com/378023/50758847-b27e3f00-12a6-11e9-8c2e-6c0bf085fdbd.gif) + ![editable](https://user-images.githubusercontent.com/378023/50885378-e71e0200-1430-11e9-98ca-6cab06b26bb4.gif) - If a diff is read-only, there will be no caret. The mouse cursor shows as "default" arrow. Selecting still works. - ![non-editable](https://user-images.githubusercontent.com/378023/50758848-b27e3f00-12a6-11e9-996c-8a458196e070.gif) - + ![non-editable](https://user-images.githubusercontent.com/378023/50885379-e71e0200-1430-11e9-8f8a-47e86bcb8090.gif) #### 2. Within an editable state - The editable state behaves almost like a normal editor, except that the green background of added lines and red background of deleted lines will be kept. From 3182a33fa5dfe85d211f628f20ba7c56a91d6771 Mon Sep 17 00:00:00 2001 From: simurai Date: Wed, 9 Jan 2019 17:12:09 +0900 Subject: [PATCH 17/17] Edit read-only --- docs/feature-requests/005-editable-diff.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/feature-requests/005-editable-diff.md b/docs/feature-requests/005-editable-diff.md index f80d6c5d55..e7c73dbc21 100644 --- a/docs/feature-requests/005-editable-diff.md +++ b/docs/feature-requests/005-editable-diff.md @@ -27,7 +27,7 @@ Currently, we use diff view in several places; which ones of them are editable? - By default, a diff view is editable (with some [exceptions](#what-is-editable) listed above). By clicking into any diff, a blinking caret will appear to signify users they can start editing. ![editable](https://user-images.githubusercontent.com/378023/50885378-e71e0200-1430-11e9-98ca-6cab06b26bb4.gif) -- If a diff is read-only, there will be no caret. The mouse cursor shows as "default" arrow. Selecting still works. +- If a diff is read-only, the caret is not visible. Selecting still works. ![non-editable](https://user-images.githubusercontent.com/378023/50885379-e71e0200-1430-11e9-8f8a-47e86bcb8090.gif) #### 2. Within an editable state @@ -36,7 +36,6 @@ Currently, we use diff view in several places; which ones of them are editable? - A deleted line will still be visible within the editable state, but it _is not editable_. That will be communicated to users in a very clear visual manner: - The cursor for hovering over deleted lines will show up as an arrow as opposed to a text cursor. - Deleted lines can still be selected (via mouse or keyboard) but no caret will be visible; basically same as current behaviour. - ![deleted](https://user-images.githubusercontent.com/378023/50759911-b3fd3680-12a9-11e9-8332-abf211e0743c.gif) - As a user makes edits in a diff view, the file name will have some visual cue indicating there are unsaved modifications, akin to when a regular atom editor has unsaved changes. - For multiple file diff views, the unsaved modification indicator will be on a per-file basis. In the following example, `src/fish.txt` has unsaved modification but `something` does not: unsaved modification indicator