Compare to Branch by iAmWillShepherd · Pull Request #4182 · desktop/desktop

iAmWillShepherd · 2018-03-06T00:03:03Z

#So, I accidentally built all the things in #3956, my bad. I was confused because I used the image in the first issue to build out my tasks list.

Fixes #3967
Fixes #3968
Fixes #3997
Fixes #3998

I also created issues #4317, #4458, and #4318 to track additional work that needs to be done around this feature.

MVP

Add feature flag
Rename Changes tab -> Commit
Rename History tab -> Compare
Add UI to select a branch
Add TabBar component to allow selecting ahead or behind
Update commit list based on CompareType
If no branch is selected we should show all commits (basically do what we've always done) and hide the ahead/behind buttons
Update ahead/behind info when branch is switched
Add a button to merge commits that are ahead of the current branch

Polish

QA

Update top-level menu to reflect new Compare and Commit tab names
~~Disable return key when no matches exist (this is harder than it seems, may need to track in a new issue)~~ (tracking in Disable return key when no matches exist #4458)
Auto-complete it too eager, we want to match character-by-character
Ahead/Behind tab bar is too sticky, when a branch is switched, default to behind tab
Any branch with commits beyond roughly 100, do not show when scrolling down
User cannot "arrow-key-down" from compare-branch filter to the search results. Nor can the user "arrow-key down" from the Behind or Ahead tab to the commits.
Behind/Ahead placement is not consistent. A/B or B/A?
when selecting a branch, the user is automatically navigated to the Behind/Ahead tabs, yet the Behind tab is not highlighted blue until the user clicks it
Odd behavior... select "origin/upgrade-yarn-time" as branch to compare. Behind/Ahead tabs shown with results as expected. Now place cursor back in search bar, and the "origin/upgrade-yarn-time" should show as a search result. Now hit enter, and notice there is nothing there. Behind/Ahead tabs should be shown.

Incomplete

[RBB] Merge changes into your branch #3998: reverting on conflict + error message

Progress

Ahead/Behind and Commits

Merging

iAmWillShepherd · 2018-03-15T01:16:13Z

I took @shiftkey's advice and went back to using a select input because it's much easier to test switching branches. The code to do auto-completion is complete, so switching back shouldn't be too much of an issue.

iAmWillShepherd · 2018-03-15T01:18:51Z

Currently working through issue where ahead/behind isn't updated for a selected branch until you click on one of the radio buttons.

iAmWillShepherd · 2018-03-16T00:13:34Z

Right now, since I am relying on the BranchList component to show branches, I am not able to render the ahead/behind info. I'm considering modifying the component to accept a BranchItem render prop so that I can customize the rendering of the list. I also updated the FilterList to accept a TextBox component. This is needed because I need the TextBox to always be visible, so I cannot rely on the one that's built into the FilterList component.

iAmWillShepherd · 2018-03-16T02:40:46Z

Most functionality is here, but this will need some CSS magic to really make it pop.

iAmWillShepherd · 2018-03-16T17:47:23Z

Branches List

The TextBox takes up too much vertical space, pushing the branch list down. Removing the CSS rule flex: 1; seems to make the element take up only the space it needs.

Commit List

The commit list should be visible directly underneath the two radio buttons. @shiftkey mentioned this is because it's a React-Virtualized component and will only render things when it has space to do so. Giving it some height in Chrome dev tools works only when I set the value to a height in pixels first (I can use height: 100% afterwards)

Cache invalidation strikes again

We weren't sure if we should extend the TextBox component, so we created a new FancyTextBox component in the spirit of moving forward Co-Authored-By: William Shepherd <iamwillshepherd@users.noreply.github.com>

Co-Authored-By: William Shepherd <iamwillshepherd@users.noreply.github.com>

Add a composite component for TextBox with icon

asRange helper function & cleanup

nerdneha

the changes look great!!! just ⛏ on the todos

app/src/ui/history/compare.tsx

+
+    if (formState.kind === ComparisonView.None) {
+      // the tab control should never be shown in this case
+      // TODO: can we enforce this with TYPES?


app/src/ui/lib/filter-list.tsx

  readonly renderNoItems?: () => JSX.Element | null
+
+  /** Used to get a reference to a TextBox that will be used to control this component */
+  readonly filterTextBox?: TextBox //Todo: don't rely on passing textbox to component


nerdneha · 2018-04-24T02:59:55Z

@shiftkey

@nerdneha because the ahead/behind stats work is proportional to the number of local and remote branches in a repository, the work to compute branches is queued up serially using requestIdleCallback - see #4399 for more context.

after reading the code, that made sense; in fact that wasn't the part that felt off when i was testing; it was the fact that the component was calculating the ahead/behind when i clicked on a compare-branch and then it felt like the branch list component hadn't "remembered". programmatically i get that it's queued and it'll show up soon but still felt weird as a user.

shiftkey · 2018-04-24T03:07:16Z

it was the fact that the component was calculating the ahead/behind when i clicked on a compare-branch and then it felt like the branch list component hadn't "remembered".

Right, now I'm with you. Mind opening a new issue and indicate that selecting a branch (which computes the ahead/behind stats) should also push that into the cache (which should then refresh the list the next time you switch to it)?

nerdneha · 2018-04-24T03:09:10Z

feeling really good about this feature and solid updates based on code review! 🎈

IMO given that @donokuda opened up a PR for some of the visual pieces and that we have an open PR for updating the way we use FilterList, I don't think there's anything that's blocking a merge. (although I'd like to get rid of that TODO if there's nothing that really needs it).

things i tested:

comparing to a branch with 0 behind and X ahead
comparing to a branch with X behind and 0 ahead
comparing to a branch with X behind and Y ahead
- merging successfully + undoing
- merging that resulted in merge conflicts, undoing the commit that caused conflict, discarding changes, and successfully merging
comparing with many branches back-to-back
filtering for branches in different ways
filtering on something not found to see "no branches" view
trying to merge when there's nothing to merge
branching from a branch + then merging back into that branch
merging between two branches
merging master into something with >2k commits

it's got a 👍 from me. ccing @niik in case there's anything he wants to comment on

saiKumarGanji · 2018-04-26T19:02:45Z

When will the exe file will be released which will have this feature

shiftkey · 2018-04-30T17:39:18Z

@saiKumarGanji once #4545 is merged and we publish an update to the beta channel it will be available there

jonnyijapan · 2019-04-03T06:40:02Z

This feature shows "only" differing commits, and then the changed files in those specific diffs. It would be helpful if Github Desktop could show the summarized diff of all the files in all of the differing commits. Like the diff when using Github.com in a pull request etc. Sorry for a late "request".

shiftkey · 2019-05-01T12:18:38Z

Sorry for a late "request".

@jonnyijapan if this is something that is still of interest to you, please open a suggestion so we can track it separate to the original work.

iAmWillShepherd modified the milestone: 1.2 Mar 6, 2018

iAmWillShepherd requested a review from joshaber March 15, 2018 15:03

iAmWillShepherd and others added 19 commits March 23, 2018 17:51

Handle esc key being pressed

fc823dc

Spelling

0c4750f

Cache compare results

769361e

Add function to get compare result

ee98987

Delegate to new compare result function

883c361

Rename file

9e2321d

Update imports

7ad926e

Add new component for rendering branches with ahead/behind info

b0dff0f

Delegate to new component

814c8b1

Lint

d458ca2

Use a string as the key

a7178ee

No caching

abf2f6d

Cache invalidation strikes again

Create Fancy Text Box component 🎩

c6b5ae9

We weren't sure if we should extend the TextBox component, so we created a new FancyTextBox component in the spirit of moving forward Co-Authored-By: William Shepherd <iamwillshepherd@users.noreply.github.com>

FancyTextStyles

909c960

Co-Authored-By: William Shepherd <iamwillshepherd@users.noreply.github.com>

Extend ITextBoxProps

3a088ee

Export so it can be used in related TextBox componenet

7385e9a

Set filter text to nothing on escape

b7fb249

Merge pull request #4316 from desktop/fancy-text-box

0a7a32a

Add a composite component for TextBox with icon

Merge branch 'master' into compare-branches

d2eeb14

niik force-pushed the compare-branches branch from f0076a1 to d2eeb14 Compare March 23, 2018 17:10

iAmWillShepherd mentioned this pull request Mar 23, 2018

Loading ahead/behind info for each branch is slow #4318

Closed

Renable merge button

b3534c6

iAmWillShepherd and others added 7 commits April 23, 2018 14:05

Merge pull request #4507 from desktop/as-range-helper-function

2ae6945

asRange helper function & cleanup

adding some docs

d3fae20

tidy up triggers for CI builds

67894b6

tidy up things

71e52ea

stray console.log

8ab5810

update placeholder text

162630a

tidy up comments

1ecb47e

nerdneha suggested changes Apr 24, 2018

View reviewed changes

shiftkey added 4 commits April 23, 2018 21:27

workaround for state issue when clearing textbox

fcfe02a

cleanup comment

d55e56d

conditionalize this work

7ab2ffa

eqeqeq

6f5e75c

update TODO to point to tech-debt issue

86a651c

nerdneha previously approved these changes Apr 24, 2018

View reviewed changes

Merge branch 'master' into compare-branches

737b0b8

nerdneha dismissed their stale review via 737b0b8 April 24, 2018 16:53

nerdneha previously approved these changes Apr 24, 2018

View reviewed changes

prettierrrrr

6a91770

shiftkey dismissed nerdneha’s stale review via 6a91770 April 24, 2018 16:58

nerdneha approved these changes Apr 24, 2018

View reviewed changes

nerdneha merged commit 2406f4b into master Apr 24, 2018

iAmWillShepherd deleted the compare-branches branch April 24, 2018 17:35

shiftkey mentioned this pull request Apr 24, 2018

add compare value into cache #4517

Merged

Conversation

iAmWillShepherd commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MVP

Polish

QA

Incomplete

Progress

Ahead/Behind and Commits

Merging

Uh oh!

iAmWillShepherd commented Mar 15, 2018

Uh oh!

iAmWillShepherd commented Mar 15, 2018

Uh oh!

iAmWillShepherd commented Mar 16, 2018

Uh oh!

iAmWillShepherd commented Mar 16, 2018

Uh oh!

iAmWillShepherd commented Mar 16, 2018

Branches List

Commit List

Uh oh!

nerdneha left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

nerdneha commented Apr 24, 2018

Uh oh!

shiftkey commented Apr 24, 2018

Uh oh!

nerdneha commented Apr 24, 2018

Uh oh!

saiKumarGanji commented Apr 26, 2018

Uh oh!

shiftkey commented Apr 30, 2018

Uh oh!

jonnyijapan commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiftkey commented May 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

iAmWillShepherd commented Mar 6, 2018 •

edited

Loading

jonnyijapan commented Apr 3, 2019 •

edited

Loading