Preferences redesign #8813

kuychaco · 2019-12-18T03:03:01Z

Closes #8774
Closes #7746

Description

A few months ago, we introduced a feature that when you have uncommitted changes, we let you either bring the changes to your new branch or leave them on your current branch. Generally this has been really well received, but some folks have specific workflows where this introduces a lot of extra clicks and pain.

We've decided to introduce a preference for this, but our preferences are getting a little overloaded so I assessed a few different ways we could introduce this new setting, including a bigger redesign to our preferences pane

This PR implements the new preference, as well as the redesign of the preferences pane.

Screenshots

Before: Tab design

After: Sidebar design

New side navigation
New switching branches preference
Reworded the confirmation dialog preferences for less redundancy
Moved integration preferences to its own new tab

Release notes

Notes: Redesign preferences pane and add preference for stashing changes when switching branches.

kuychaco · 2019-12-27T22:23:10Z

For when y'all get back from the holidays... 🙏

@ampinsk would love a design review from you! Feel free to make modifications as you see fit (commits are welcome!)

@tierninho would appreciate your QA on this!

kuychaco · 2019-12-27T22:34:02Z

@ampinsk is it desired/expected to have that salmon background color for the merge tool section? It's from a brutalism class

niik · 2020-01-02T16:14:32Z

@ampinsk is it desired/expected to have that salmon background color for the merge tool section? It's from a brutalism class

@kuychaco That style is one we used to apply in the early days when designs were not finalized yet. @joshaber dubbed it "pink brutalism" and the thinking was that by applying that style a developer could trigger something deep and unsettling within myself or @donokuda and said discomfort would result in us springing into action and finalizing the actual designs before anyone else would see the abhorrent injustice afflicted on the otherwise perfect piece of design that was the Desktop app.

In this case the merge tool selection is behind a dev feature flag because we never ended up shipping it. At some point it might be a good idea to unship all of #2802 since it's been superseded by the in-app conflict resolution work.

outofambit · 2020-01-02T18:14:51Z

@niik @kuychaco just chiming in to say +1 for removing the merge tool ui from the codebase entirely!

donokuda · 2020-01-02T19:33:16Z

[...] discomfort would result in us springing into action and finalizing the actual designs before anyone else would see the abhorrent injustice afflicted on the otherwise perfect piece of design that was the Desktop app.

Poppin' in here to say that the plan backfired and I ended up embracing pink brutalism 😍

tierninho

Here are my test results on this feature:

No way to tab up/down the menu items
Padding here looks very narrow between the button and bottom of the window. Maybe add some?
Selecting the "Always stash on current branch" option fails to warn the user if a stash is already on that branch.
On multiple occasions, the "Always stash on current branch" option resulted in losing the stashed files. After files were stashed, I selected a few other branches and came back to branch with stashed files and no where to be found.
Overall the stashing updates were not working correctly. I select "ask me" option, then on the first attempt it asks me, but second it attempt it does not -- the changes are brought over without the prompt due to a protected branch.
Line is not the same width

ampinsk · 2020-01-07T21:50:56Z

Thank you @kuychaco this is looking so good! 💕 A couple of tiny things I noticed that I wasn't sure how to fix on my own:

In this screenshot:

Looks like the text isn't lining up on the left (you can see based on the red line)
The non-selected tab icons should be gray

In this screenshot:

The background color behind the sidebar tabs in dark mode is darker than the tabs, they should be the same dark gray

Before	After

For advanced settings, is it possible to get the spacing closer to the screenshot on the right? I tightened the spacing between the row items and increased the spacing between the sections

Let me know if you want to pair on any of this! ✨

Co-Authored-By: evelyn masso <outofambit@github.com>

Co-Authored-By: Amanda Pinsker <apinsker4@gmail.com>

Co-Authored-By: evelyn masso <outofambit@github.com> Co-Authored-By: Amanda Pinsker <apinsker4@gmail.com>

kuychaco · 2020-01-10T01:58:21Z

Okay! Styling fixes complete! Thanks @outofambit and @ampinsk for your great input 🙏

@ampinsk how does this look for the Advanced section... we can pair if you want to further tweak the spacing. Or you can make changes to the values I used in 5bbc79d

Current State	Your Design

Otherwise I think this is ready for a full code review! /cc @outofambit

ampinsk · 2020-01-10T02:39:20Z

That spacing looks great!!

Just one last thing I noticed from your screenshot is the non-selected icons should be gray 😄 Otherwise this seems ready to go!

Co-Authored-By: Amanda Pinsker <apinsker4@gmail.com>

outofambit

left some initial thoughts, mostly around types. still need to review the .tsx files more closely.

outofambit · 2020-01-13T19:09:56Z

app/src/lib/stores/app-store.ts

@@ -1769,6 +1777,12 @@ export class AppStore extends TypedBaseStore<IAppState> {
      askForConfirmationOnForcePushDefault
    )

+    const strategy = localStorage.getItem(
+      uncommittedChangesStrategyKindKey
+    ) as UncommittedChangesStrategyKind


I'd really like to avoid using as here, though I'm not sure of a good alternative approach yet. localStorage.getItem can return undefined if the given key doesn't exist, and this implementation tells the compiler that 'undefined' is a valid UncommittedChangesStrategyKind, which will result in runtime errors.

outofambit · 2020-01-13T19:14:51Z

app/src/lib/stores/app-store.ts

+      if (hasChanges) {
+        if (uncommittedChangesStrategy.kind === askToStash.kind) {
+          this._showPopup({
+            type: PopupType.StashAndSwitchBranch,
+            branchToCheckout: foundBranch,
+            repository,
+          })
+          return repository
+        }


outofambit · 2020-01-13T19:18:03Z

app/src/models/uncommitted-changes-strategy.ts

+export function getUncommittedChangesStrategy(
+  kind: UncommittedChangesStrategyKind
+) {
+  switch (kind) {
+    case UncommittedChangesStrategyKind.AskForConfirmation:
+      return askToStash
+    case UncommittedChangesStrategyKind.MoveToNewBranch:
+      return moveToNewBranch
+    case UncommittedChangesStrategyKind.StashOnCurrentBranch:
+      return stashOnCurrentBranch
+    default:
+      return assertNever(
+        kind,
+        `Unknown UncommittedChangesStrategyKind: ${kind}`
+      )
+  }
+}


I have some concerns about this function, but I think the core of my concern is that it took me a while to figure out what it was doing. Perhaps documenting this to clarify that its primary purpose is to map a kind to an actual UncommittedChangesStrategy?

It feels to me like there's a simpler version of this overall approach, but I'm not sure what it is yet...Is the main reason this exists so that we can store an UncommittedChangesStrategyKind in AppState and convert it to an UncommittedChangesStrategy whenever we use it? perhaps we could store the result of this function in AppState and avoid doing this conversion in many places?

outofambit

a few more comments, nothing non-blocking. react components and styling look good to me!

outofambit · 2020-01-14T22:08:26Z

app/src/ui/branches/branches-container.tsx

+
+  readonly selectedUncommittedChangesStrategy: UncommittedChangesStrategy
+
+  readonly hasChangesAndStash: boolean


non-blocking idea for a maybe more legible name:

Suggested change

readonly hasChangesAndStash: boolean

readonly couldOverwriteStash: boolean

outofambit · 2020-01-14T22:18:18Z

app/src/ui/tab-bar.tsx

+          (type === TabBarType.Switch
+            ? 'switch'
+            : type === TabBarType.Vertical
+            ? 'vertical'
+            : 'tabs')


would be nice to pull this out into a little switch...case with an assertNever, but not a big deal. (non-blocking)

outofambit · 2020-01-14T22:19:30Z

app/src/ui/toolbar/branch-dropdown.tsx

+
+  readonly selectedUncommittedChangesStrategy: UncommittedChangesStrategy
+
+  readonly hasChangesAndStash: boolean


same thing as above regarding variable name

useful for getting values out of local storage Co-Authored-By: Katrina Uychaco <katrina@github.com>

Co-Authored-By: Katrina Uychaco <katrina@github.com>

Co-Authored-By: evelyn masso <outofambit@github.com>

…-redesign

kuychaco added 2 commits December 17, 2019 18:59

Separate out Integrations tab

27532d8

Include footer for Integrations tab

b9be775

kuychaco added this to In Progress PRs in Desktop 2.3 release via automation Dec 18, 2019

kuychaco added 17 commits December 18, 2019 18:03

Add "Applications" header to Integrations tab

feddfad

Make Advanced tab less redundant

be54b99

Add section in Advanced for handling changes when switching branches

222b8b8

Wire up radio buttons, store value in local storage

a397784

Add getUncommittedChangesStrategy helper

54eb1c9

Default to selected uncommittedChangesStrategy in app store methods

74a6408

Use uncommittedChangesStragety selected when switching branches

fe54a35

Use uncommittedChangesStrategy selected when creating new branch

b89ecbd

Verticalize tab bar

a9b202b

Add icons

a83f84a

Make dialog width wider for new sidebar

e37b78b

Update styles for selected item

77af3a4

Style icons

ca3d5ba

Fix spacing

211f5a0

Border radius

14fd997

Fix width of tab-container

13e7516

Merge branch 'development' into ku-preferences-redesign

ff7cd2c

kuychaco marked this pull request as ready for review December 27, 2019 22:21

kuychaco requested review from tierninho and ampinsk December 27, 2019 22:23

kuychaco moved this from In Progress PRs to Awaiting Review in Desktop 2.3 release Dec 27, 2019

tierninho reviewed Jan 3, 2020

View reviewed changes

kuychaco and others added 6 commits January 8, 2020 13:37

Styling tweaks

a5454b0

Co-Authored-By: evelyn masso <outofambit@github.com>

Extract styles to shared base class

d7bed4e

Co-Authored-By: evelyn masso <outofambit@github.com>

Fix tab bar background color

4cd70f2

Co-Authored-By: Amanda Pinsker <apinsker4@gmail.com>

Align text next to icons

7e50c7d

Co-Authored-By: evelyn masso <outofambit@github.com> Co-Authored-By: Amanda Pinsker <apinsker4@gmail.com>

Fix spacing of Advanced section

5bbc79d

Merge branch 'development' into ku-preferences-redesign

3acdd9a

kuychaco and others added 3 commits January 10, 2020 12:46

Make non-selected icons gray

48d3a65

Co-Authored-By: Amanda Pinsker <apinsker4@gmail.com>

Merge branch 'development' into ku-preferences-redesign

1a56994

🔥 unnecessary code

77354a6

outofambit self-requested a review January 11, 2020 00:32

Rexogamer mentioned this pull request Jan 12, 2020

remove the merge tool #8896

Merged

outofambit reviewed Jan 13, 2020

View reviewed changes

outofambit reviewed Jan 14, 2020

View reviewed changes

outofambit and others added 5 commits January 14, 2020 16:40

add a parse method for uncommittedChangesStrategyKind

fe21309

useful for getting values out of local storage Co-Authored-By: Katrina Uychaco <katrina@github.com>

some docs for getUncommittedChangesStrategy

f2aeb50

Co-Authored-By: Katrina Uychaco <katrina@github.com>

remove extra line

09e954c

Co-Authored-By: Katrina Uychaco <katrina@github.com>

hasChagnesAndStash --> couldOverwriteStash

8cee164

Co-Authored-By: evelyn masso <outofambit@github.com>

Merge branch 'development' into ku-preferences-redesign

6d70d14

outofambit previously approved these changes Jan 15, 2020

View reviewed changes

lint

564be7f

outofambit dismissed their stale review via 564be7f January 15, 2020 17:02

Merge remote-tracking branch 'origin/development' into ku-preferences…

79dc0d4

…-redesign

outofambit approved these changes Jan 15, 2020

View reviewed changes

outofambit merged commit 91ecdb5 into development Jan 15, 2020

Desktop 2.3 release automation moved this from Review In Progress to PRs For Next Beta Jan 15, 2020

outofambit deleted the ku-preferences-redesign branch January 17, 2020 18:39

niik mentioned this pull request Nov 2, 2020

GitHub Desktop may overwrite existing stash when "Always stash and leave my changes" is selected #10956

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preferences redesign #8813

Preferences redesign #8813

kuychaco commented Dec 18, 2019 •

edited

kuychaco commented Dec 27, 2019

kuychaco commented Dec 27, 2019

niik commented Jan 2, 2020

outofambit commented Jan 2, 2020

donokuda commented Jan 2, 2020

tierninho left a comment •

edited

ampinsk commented Jan 7, 2020 •

edited

kuychaco commented Jan 10, 2020

ampinsk commented Jan 10, 2020

outofambit left a comment

outofambit Jan 13, 2020

outofambit Jan 13, 2020

outofambit Jan 13, 2020

outofambit left a comment

outofambit Jan 14, 2020

outofambit Jan 14, 2020

outofambit Jan 14, 2020


		readonly selectedUncommittedChangesStrategy: UncommittedChangesStrategy

		readonly hasChangesAndStash: boolean

	readonly hasChangesAndStash: boolean
	readonly couldOverwriteStash: boolean

Preferences redesign #8813

Preferences redesign #8813

Conversation

kuychaco commented Dec 18, 2019 • edited

Description

Screenshots

Release notes

kuychaco commented Dec 27, 2019

kuychaco commented Dec 27, 2019

niik commented Jan 2, 2020

outofambit commented Jan 2, 2020

donokuda commented Jan 2, 2020

tierninho left a comment • edited

Choose a reason for hiding this comment

ampinsk commented Jan 7, 2020 • edited

kuychaco commented Jan 10, 2020

ampinsk commented Jan 10, 2020

outofambit left a comment

Choose a reason for hiding this comment

outofambit Jan 13, 2020

Choose a reason for hiding this comment

outofambit Jan 13, 2020

Choose a reason for hiding this comment

outofambit Jan 13, 2020

Choose a reason for hiding this comment

outofambit left a comment

Choose a reason for hiding this comment

outofambit Jan 14, 2020

Choose a reason for hiding this comment

outofambit Jan 14, 2020

Choose a reason for hiding this comment

outofambit Jan 14, 2020

Choose a reason for hiding this comment

kuychaco commented Dec 18, 2019 •

edited

tierninho left a comment •

edited

ampinsk commented Jan 7, 2020 •

edited