Skip to content

feat: support toggling 'whole file' annotations#149

Merged
tjkandala merged 8 commits intomasterfrom
toggle-file-annotations
Oct 23, 2020
Merged

feat: support toggling 'whole file' annotations#149
tjkandala merged 8 commits intomasterfrom
toggle-file-annotations

Conversation

@tjkandala
Copy link
Copy Markdown
Contributor

The toggle action now cycles through 'none' (no annotations), 'line' (annotations only for selected lines) and 'file' (annotations for all lines).

I tried to make this as backwards compatible as possible, but the first toggle for existing users may be a bit awkward.

@tjkandala tjkandala requested a review from a team October 19, 2020 20:57
@felixfbecker
Copy link
Copy Markdown
Contributor

Is this the best UX? I wonder what is more common:

  1. Switching between one-line vs all-lines
  2. Switching between on and off (any decorations or no decorations)

If (2) is common, and (1) is rather rare, we should decouple the settings, keep (2) as the sole function of the toggle and allow switching (1) through a command in the command palette and/or through the altAction of the button. We also can explain the alt action in the tooltip.

I'm personally leaning towards that, thoughts @sourcegraph/web @AlicjaSuska ?

@AlicjaSuska
Copy link
Copy Markdown

I really like this change of cycling through 3 states. I think it's relatively easy to learn. After the first use, there should be no problem imo.

I think that your solution @felixfbecker is ok as well but it may be way harder to discover. I would go with @tjkandala 's change 👍

Comment thread package.json
"type": "boolean",
"default": false
},
"git.blame.decorations": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The other setting should be removed, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@AlicjaSuska
Copy link
Copy Markdown

@tjkandala there is a bug: when the extension is 'off' is shouldn't be highlighted anymore. Shown at the end of the video:

https://www.loom.com/share/e650ef2d8dcf422a83aae74707138e50

@tjkandala
Copy link
Copy Markdown
Contributor Author

@tjkandala there is a bug: when the extension is 'off' is shouldn't be highlighted anymore. Shown at the end of the video:

Thanks for spotting this, I think it's because I kept references to the old settings in an attempt to be backwards compatible, but that is a lot more trouble than it's worth. I'll simplify the settings and this should go away

@felixfbecker
Copy link
Copy Markdown
Contributor

@tjkandala just make sure that the visual pressed state stays correct even for users who were users of git blame before and that it defaults to "line" (on). I think it's fine to ignore any old setting.

@felixfbecker
Copy link
Copy Markdown
Contributor

@tjkandala checking in - are you working on the fix for the problem @AlicjaSuska pointed out?

@tjkandala
Copy link
Copy Markdown
Contributor Author

Update: "onboarding" users with deprecated settings more gracefully now. Update configuration based on deprecated settings when the new setting, git.blame.decorations, is not set.

@tjkandala tjkandala merged commit 2e80397 into master Oct 23, 2020
@tjkandala tjkandala deleted the toggle-file-annotations branch October 23, 2020 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants