Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preferences redesign #8813

Merged
merged 42 commits into from
Jan 15, 2020
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
27532d8
Separate out Integrations tab
kuychaco Dec 18, 2019
b9be775
Include footer for Integrations tab
kuychaco Dec 18, 2019
feddfad
Add "Applications" header to Integrations tab
kuychaco Dec 19, 2019
be54b99
Make Advanced tab less redundant
kuychaco Dec 19, 2019
222b8b8
Add section in Advanced for handling changes when switching branches
kuychaco Dec 19, 2019
a397784
Wire up radio buttons, store value in local storage
kuychaco Dec 21, 2019
54eb1c9
Add getUncommittedChangesStrategy helper
kuychaco Dec 21, 2019
74a6408
Default to selected uncommittedChangesStrategy in app store methods
kuychaco Dec 21, 2019
fe54a35
Use uncommittedChangesStragety selected when switching branches
kuychaco Dec 21, 2019
b89ecbd
Use uncommittedChangesStrategy selected when creating new branch
kuychaco Dec 21, 2019
a9b202b
Verticalize tab bar
kuychaco Dec 25, 2019
a83f84a
Add icons
kuychaco Dec 27, 2019
e37b78b
Make dialog width wider for new sidebar
kuychaco Dec 27, 2019
77af3a4
Update styles for selected item
kuychaco Dec 27, 2019
ca3d5ba
Style icons
kuychaco Dec 27, 2019
211f5a0
Fix spacing
kuychaco Dec 27, 2019
14fd997
Border radius
kuychaco Dec 27, 2019
13e7516
Fix width of tab-container
kuychaco Dec 27, 2019
ff7cd2c
Merge branch 'development' into ku-preferences-redesign
kuychaco Dec 27, 2019
c97f005
Fix styling
kuychaco Jan 6, 2020
3c72edd
Use arrow up/down for navigation
kuychaco Jan 6, 2020
76afcf6
Stash only if changes exist
kuychaco Jan 6, 2020
eeb0589
Prompt before overwriting current stash
kuychaco Jan 7, 2020
bc7c047
Only prompt if there are changes
kuychaco Jan 7, 2020
a60d295
Merge branch 'development' into ku-preferences-redesign
kuychaco Jan 7, 2020
4c5ac1d
Tiny spacing tweaks
ampinsk Jan 7, 2020
a5454b0
Styling tweaks
kuychaco Jan 8, 2020
d7bed4e
Extract styles to shared base class
kuychaco Jan 8, 2020
4cd70f2
Fix tab bar background color
kuychaco Jan 8, 2020
7e50c7d
Align text next to icons
kuychaco Jan 8, 2020
5bbc79d
Fix spacing of Advanced section
kuychaco Jan 10, 2020
3acdd9a
Merge branch 'development' into ku-preferences-redesign
kuychaco Jan 10, 2020
48d3a65
Make non-selected icons gray
kuychaco Jan 10, 2020
1a56994
Merge branch 'development' into ku-preferences-redesign
kuychaco Jan 10, 2020
77354a6
:fire: unnecessary code
kuychaco Jan 10, 2020
fe21309
add a parse method for uncommittedChangesStrategyKind
outofambit Jan 15, 2020
f2aeb50
some docs for getUncommittedChangesStrategy
outofambit Jan 15, 2020
09e954c
remove extra line
outofambit Jan 15, 2020
8cee164
hasChagnesAndStash --> couldOverwriteStash
kuychaco Jan 15, 2020
6d70d14
Merge branch 'development' into ku-preferences-redesign
kuychaco Jan 15, 2020
564be7f
lint
outofambit Jan 15, 2020
79dc0d4
Merge remote-tracking branch 'origin/development' into ku-preferences…
outofambit Jan 15, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/src/lib/app-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import { GitRebaseProgress } from '../models/rebase'
import { RebaseFlowStep } from '../models/rebase-flow-step'
import { IStashEntry } from '../models/stash-entry'
import { TutorialStep } from '../models/tutorial-step'
import { UncommittedChangesStrategyKind } from '../models/uncommitted-changes-strategy'

export enum SelectionType {
Repository,
Expand Down Expand Up @@ -174,6 +175,9 @@ export interface IAppState {
/** Should the app prompt the user to confirm a force push? */
readonly askForConfirmationOnForcePush: boolean

/** How the app should handle uncommitted changes when switching branches */
readonly uncommittedChangesStrategyKind: UncommittedChangesStrategyKind

/** The external editor to use when opening repositories */
readonly selectedExternalEditor: ExternalEditor | null

Expand Down
66 changes: 51 additions & 15 deletions app/src/lib/stores/app-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,8 @@ import {
import {
UncommittedChangesStrategy,
UncommittedChangesStrategyKind,
uncommittedChangesStrategyKindDefault,
getUncommittedChangesStrategy,
askToStash,
} from '../../models/uncommitted-changes-strategy'
import { IStashEntry, StashedChangesLoadStates } from '../../models/stash-entry'
Expand Down Expand Up @@ -281,6 +283,9 @@ const confirmRepoRemovalKey: string = 'confirmRepoRemoval'
const confirmDiscardChangesKey: string = 'confirmDiscardChanges'
const confirmForcePushKey: string = 'confirmForcePush'

const uncommittedChangesStrategyKindKey: string =
'uncommittedChangesStrategyKind'

const externalEditorKey: string = 'externalEditor'

const imageDiffTypeDefault = ImageDiffType.TwoUp
Expand Down Expand Up @@ -364,6 +369,8 @@ export class AppStore extends TypedBaseStore<IAppState> {
private imageDiffType: ImageDiffType = imageDiffTypeDefault
private hideWhitespaceInDiff: boolean = hideWhitespaceInDiffDefault

private uncommittedChangesStrategyKind: UncommittedChangesStrategyKind = uncommittedChangesStrategyKindDefault

private selectedExternalEditor: ExternalEditor | null = null

private resolvedExternalEditor: ExternalEditor | null = null
Expand Down Expand Up @@ -705,6 +712,7 @@ export class AppStore extends TypedBaseStore<IAppState> {
.askForConfirmationOnRepositoryRemoval,
askForConfirmationOnDiscardChanges: this.confirmDiscardChanges,
askForConfirmationOnForcePush: this.askForConfirmationOnForcePush,
uncommittedChangesStrategyKind: this.uncommittedChangesStrategyKind,
selectedExternalEditor: this.selectedExternalEditor,
imageDiffType: this.imageDiffType,
hideWhitespaceInDiff: this.hideWhitespaceInDiff,
Expand Down Expand Up @@ -1793,6 +1801,12 @@ export class AppStore extends TypedBaseStore<IAppState> {
askForConfirmationOnForcePushDefault
)

const strategy = localStorage.getItem(
uncommittedChangesStrategyKindKey
) as UncommittedChangesStrategyKind
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.uncommittedChangesStrategyKind =
strategy || uncommittedChangesStrategyKindDefault

this.updateSelectedExternalEditor(
await this.lookupSelectedExternalEditor()
).catch(e => log.error('Failed resolving current editor at startup', e))
Expand Down Expand Up @@ -2968,7 +2982,9 @@ export class AppStore extends TypedBaseStore<IAppState> {
repository: Repository,
name: string,
startPoint: string | null,
uncommittedChangesStrategy: UncommittedChangesStrategy = askToStash
uncommittedChangesStrategy: UncommittedChangesStrategy = getUncommittedChangesStrategy(
this.uncommittedChangesStrategyKind
)
): Promise<Repository> {
const gitStore = this.gitStoreCache.get(repository)
const branch = await gitStore.performFailableOperation(() =>
Expand Down Expand Up @@ -3038,7 +3054,9 @@ export class AppStore extends TypedBaseStore<IAppState> {
public async _checkoutBranch(
repository: Repository,
branch: Branch | string,
uncommittedChangesStrategy: UncommittedChangesStrategy = askToStash
uncommittedChangesStrategy: UncommittedChangesStrategy = getUncommittedChangesStrategy(
this.uncommittedChangesStrategyKind
)
): Promise<Repository> {
const gitStore = this.gitStoreCache.get(repository)
const kind = 'checkout'
Expand All @@ -3058,20 +3076,22 @@ export class AppStore extends TypedBaseStore<IAppState> {
let stashToPop: IStashEntry | null = null
if (enableStashing()) {
const hasChanges = changesState.workingDirectory.files.length > 0
if (hasChanges && uncommittedChangesStrategy.kind === askToStash.kind) {
this._showPopup({
type: PopupType.StashAndSwitchBranch,
branchToCheckout: foundBranch,
if (hasChanges) {
if (uncommittedChangesStrategy.kind === askToStash.kind) {
this._showPopup({
type: PopupType.StashAndSwitchBranch,
branchToCheckout: foundBranch,
repository,
})
return repository
}
Comment on lines +3056 to +3064
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😋


stashToPop = await this.stashToPopAfterBranchCheckout(
repository,
})
return repository
foundBranch,
uncommittedChangesStrategy
)
}

stashToPop = await this.stashToPopAfterBranchCheckout(
repository,
foundBranch,
uncommittedChangesStrategy
)
}

const checkoutSucceeded =
Expand Down Expand Up @@ -3161,7 +3181,9 @@ export class AppStore extends TypedBaseStore<IAppState> {
private async stashToPopAfterBranchCheckout(
repository: Repository,
branch: Branch,
uncommittedChangesStrategy: UncommittedChangesStrategy = askToStash
uncommittedChangesStrategy: UncommittedChangesStrategy = getUncommittedChangesStrategy(
this.uncommittedChangesStrategyKind
)
): Promise<IStashEntry | null> {
const {
changesState,
Expand Down Expand Up @@ -4446,6 +4468,20 @@ export class AppStore extends TypedBaseStore<IAppState> {
return Promise.resolve()
}

public _setUncommittedChangesStrategyKindSetting(
value: UncommittedChangesStrategyKind
): Promise<void> {
this.uncommittedChangesStrategyKind = value

localStorage.setItem(uncommittedChangesStrategyKindKey, value)

// Do we need to do this?
// this.updateMenuLabelsForSelectedRepository()

this.emitUpdate()
return Promise.resolve()
}

public _setExternalEditor(selectedEditor: ExternalEditor) {
const promise = this.updateSelectedExternalEditor(selectedEditor)
localStorage.setItem(externalEditorKey, selectedEditor)
Expand Down
7 changes: 4 additions & 3 deletions app/src/models/preferences.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export enum PreferencesTab {
Accounts = 0,
Git = 1,
Appearance = 2,
Advanced = 3,
Integrations = 1,
Git = 2,
Appearance = 3,
Advanced = 4,
}
26 changes: 26 additions & 0 deletions app/src/models/uncommitted-changes-strategy.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import { IStashEntry } from './stash-entry'
import { assertNever } from '../lib/fatal-error'

export enum UncommittedChangesStrategyKind {
AskForConfirmation = 'AskForConfirmation',
StashOnCurrentBranch = 'StashOnCurrentBranch',
MoveToNewBranch = 'MoveToNewBranch',
}

export const uncommittedChangesStrategyKindDefault: UncommittedChangesStrategyKind =
UncommittedChangesStrategyKind.AskForConfirmation

export type UncommittedChangesStrategy =
| { kind: UncommittedChangesStrategyKind.AskForConfirmation }
| { kind: UncommittedChangesStrategyKind.StashOnCurrentBranch }
Expand All @@ -20,3 +24,25 @@ export const askToStash: UncommittedChangesStrategy = {
export const stashOnCurrentBranch: UncommittedChangesStrategy = {
kind: UncommittedChangesStrategyKind.StashOnCurrentBranch,
}
export const moveToNewBranch: UncommittedChangesStrategy = {
kind: UncommittedChangesStrategyKind.MoveToNewBranch,
transientStashEntry: null,
}

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}`
)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

15 changes: 14 additions & 1 deletion app/src/ui/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ import { enableTutorial } from '../lib/feature-flag'
import { ConfirmExitTutorial } from './tutorial'
import { TutorialStep, isValidTutorialStep } from '../models/tutorial-step'
import { WorkflowPushRejectedDialog } from './workflow-push-rejected/workflow-push-rejected'
import { getUncommittedChangesStrategy } from '../models/uncommitted-changes-strategy'

const MinuteInMilliseconds = 1000 * 60
const HourInMilliseconds = MinuteInMilliseconds * 60
Expand Down Expand Up @@ -1328,6 +1329,9 @@ export class App extends React.Component<IAppProps, IAppState> {
this.state.askForConfirmationOnDiscardChanges
}
confirmForcePush={this.state.askForConfirmationOnForcePush}
uncommittedChangesStrategyKind={
this.state.uncommittedChangesStrategyKind
}
selectedExternalEditor={this.state.selectedExternalEditor}
optOutOfUsageTracking={this.state.optOutOfUsageTracking}
enterpriseAccount={this.getEnterpriseAccount()}
Expand Down Expand Up @@ -1443,6 +1447,9 @@ export class App extends React.Component<IAppProps, IAppState> {
dispatcher={this.props.dispatcher}
initialName={popup.initialName || ''}
currentBranchProtected={currentBranchProtected}
selectedUncommittedChangesStrategy={getUncommittedChangesStrategy(
this.state.uncommittedChangesStrategyKind
)}
/>
)
}
Expand Down Expand Up @@ -2319,7 +2326,9 @@ export class App extends React.Component<IAppProps, IAppState> {
currentFoldout !== null && currentFoldout.type === FoldoutType.Branch

const repository = selection.repository
const branchesState = selection.state.branchesState
const { branchesState, changesState } = selection.state
const hasAssociatedStash = changesState.stashEntry !== null
const hasChanges = changesState.workingDirectory.files.length > 0

return (
<BranchDropdown
Expand All @@ -2335,6 +2344,10 @@ export class App extends React.Component<IAppProps, IAppState> {
shouldNudge={
this.state.currentOnboardingTutorialStep === TutorialStep.CreateBranch
}
selectedUncommittedChangesStrategy={getUncommittedChangesStrategy(
this.state.uncommittedChangesStrategyKind
)}
hasChangesAndStash={hasChanges && hasAssociatedStash}
/>
)
}
Expand Down
30 changes: 27 additions & 3 deletions app/src/ui/branches/branches-container.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { startTimer } from '../lib/timing'
import {
UncommittedChangesStrategyKind,
UncommittedChangesStrategy,
askToStash,
stashOnCurrentBranch,
} from '../../models/uncommitted-changes-strategy'

interface IBranchesContainerProps {
Expand All @@ -45,6 +45,10 @@ interface IBranchesContainerProps {
readonly isLoadingPullRequests: boolean

readonly currentBranchProtected: boolean

readonly selectedUncommittedChangesStrategy: UncommittedChangesStrategy

readonly hasChangesAndStash: boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking idea for a maybe more legible name:

Suggested change
readonly hasChangesAndStash: boolean
readonly couldOverwriteStash: boolean

}

interface IBranchesContainerState {
Expand Down Expand Up @@ -241,9 +245,29 @@ export class BranchesContainer extends React.Component<
private onBranchItemClick = (branch: Branch) => {
this.props.dispatcher.closeFoldout(FoldoutType.Branch)

const { currentBranch, repository, currentBranchProtected } = this.props
const {
currentBranch,
repository,
currentBranchProtected,
dispatcher,
hasChangesAndStash,
} = this.props

if (currentBranch == null || currentBranch.name !== branch.name) {
if (
!currentBranchProtected &&
this.props.selectedUncommittedChangesStrategy.kind ===
stashOnCurrentBranch.kind &&
hasChangesAndStash
) {
dispatcher.showPopup({
type: PopupType.ConfirmOverwriteStash,
repository,
branchToCheckout: branch,
})
return
}

const timer = startTimer('checkout branch from list', repository)

// Never prompt to stash changes if someone is switching away from a protected branch
Expand All @@ -252,7 +276,7 @@ export class BranchesContainer extends React.Component<
kind: UncommittedChangesStrategyKind.MoveToNewBranch,
transientStashEntry: null,
}
: askToStash
: this.props.selectedUncommittedChangesStrategy

this.props.dispatcher
.checkoutBranch(repository, branch, strategy)
Expand Down
4 changes: 2 additions & 2 deletions app/src/ui/create-branch/create-branch-dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import { startTimer } from '../lib/timing'
import {
UncommittedChangesStrategy,
UncommittedChangesStrategyKind,
askToStash,
} from '../../models/uncommitted-changes-strategy'

interface ICreateBranchProps {
Expand All @@ -39,6 +38,7 @@ interface ICreateBranchProps {
readonly allBranches: ReadonlyArray<Branch>
readonly initialName: string
readonly currentBranchProtected: boolean
readonly selectedUncommittedChangesStrategy: UncommittedChangesStrategy
}

interface ICreateBranchState {
Expand Down Expand Up @@ -303,7 +303,7 @@ export class CreateBranch extends React.Component<
kind: UncommittedChangesStrategyKind.MoveToNewBranch,
transientStashEntry: null,
}
: askToStash
: this.props.selectedUncommittedChangesStrategy

this.setState({ isCreatingBranch: true })
const timer = startTimer('create branch', repository)
Expand Down
14 changes: 13 additions & 1 deletion app/src/ui/dispatcher/dispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,10 @@ import {
StatusCallBack,
} from '../../lib/stores/commit-status-store'
import { MergeResult } from '../../models/merge'
import { UncommittedChangesStrategy } from '../../models/uncommitted-changes-strategy'
import {
UncommittedChangesStrategy,
UncommittedChangesStrategyKind,
} from '../../models/uncommitted-changes-strategy'
import { RebaseFlowStep, RebaseStep } from '../../models/rebase-flow-step'
import { IStashEntry } from '../../models/stash-entry'

Expand Down Expand Up @@ -1477,6 +1480,15 @@ export class Dispatcher {
return this.appStore._setConfirmDiscardChangesSetting(value)
}

/**
* Sets the user's preference for handling uncommitted changes when switching branches
*/
public setUncommittedChangesStrategyKindSetting(
value: UncommittedChangesStrategyKind
): Promise<void> {
return this.appStore._setUncommittedChangesStrategyKindSetting(value)
}

/**
* Sets the user's preference for an external program to open repositories in.
*/
Expand Down
Loading