Blame UI improvements #44287

philipp-spiess · 2022-11-11T14:29:37Z

This improves the git blame view based on the design input from: https://www.figma.com/file/ecHi31BUCccGtjZsOrNR56/Git-blame-improvements?node-id=1816%3A3628

I made one substantial change from the final Figma: Instead of the zebra-striped background pattern, I decided to implement an earlier revision and use a border to visually distinguish blame hunks. The reason for this has to do with the streaming implementation of git blame that we're currently testing for some customers: In that implementation, chunks are loaded lazily and we do a first render pass with incomplete chunk data. Because of this, we don't know how many chunks appear above a rendered line and thus the zebra calculation is not possible (we don't know if we're on an even or odd chunk). IMO, changing to a border is the easiest way to fix this. We can always change this later though.

Test plan

Light mode	Dark mode

I've tested this with the none-CodeMirror view as well. The UI for that is not awesome but it's a reasonable fallback. The next release enables code mirror by default anyways.:

Screenshot

App preview:

Web

Check out the client app preview documentation to learn more.

sg-e2e-regression-test-bob · 2022-11-11T14:37:26Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (+0.05 kb)	-0.09% (-13.80 kb) 🔽	-0.12% (-13.85 kb) 🔽	0.66% (+5) 🔺

Look at the Statoscope report for a full comparison between the commits 8e8c772 and ef9aa16 or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

philipp-spiess · 2022-12-21T15:39:15Z

client/web/src/repo/blame/useBlameHunks.ts

@jhchabran Do we have a list of changes needed before we roll out the git streaming more broadly? Adding support for the user query would be necessary as well.

@philipp-spiess catching up with GH as I'm back from PTO. It seems that the new code handles this differently, is this comment still relevant?

@jhchabran Yeah this is still required. With the GraphQL query it's possible to load the user from a git author but with our streaming endpoint this is not included yet. Do you think it would be a lot of work to add this to the JSON payloads?

philipp-spiess · 2022-12-21T15:39:55Z

cmd/frontend/graphqlbackend/repository.go

@jhchabran @mrnugget I’m unsure about the authz.DefaultSubRepoPermsChecker field here. Do you have any advice on what I should be doing? 😬

Okay I verified that the other code path to resolve commits use the same thing:

https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/cmd/frontend/graphqlbackend/git_commit.go?L94

Yep. cc @sourcegraph/iam

client/web/src/repo/blob/codemirror/blame-decorations.tsx

taras-yemets · 2022-12-21T16:13:00Z

client/web/src/repo/blob/codemirror/blame-decorations.tsx

Suggested change

// We can not access the light theme from the view props beacuse we need

// We can not access the light theme from the view props because we need

taras-yemets

Awesome! These are huge improvements!

taras-yemets · 2022-12-21T16:19:18Z

I think we should update the 'old' table blob view styling to look similar to the CodeMirror blob: the blame section seems a bit condensed, recency indicators overflow sibling lines, and hunks separators are not visible.

philipp-spiess · 2022-12-21T16:22:23Z

@taras-yemets Yeah I wrote about this in the PR test plan. I don't want to invest too much into the old view given that Code Mirror is going to be the default with the same release as this change anyway. Does this make sense?

taras-yemets · 2022-12-21T16:23:26Z

I don't want to invest too much into the old view given that Code Mirror is going to be the default with the same release as this change anyway. Does this make sense?

@philipp-spiess, I somehow missed it, sorry.

philipp-spiess · 2022-12-21T16:26:42Z

@taras-yemets No worries! What are your thought? I assume that maybe a few users could be left on the old view until we remove the code in the following release. For them the experience would be subpar but also not horribly broken. And they can always enable CodeMirror to fix it and get a lot of other improvements like faster rendering

taras-yemets · 2022-12-21T16:27:48Z

Now that we have these awesome hunks separators I think it would be nice to highlight the line code content when the corresponding blame hunk is hovered (even if it's not the first line of the hunk) similar to JetBrains and VSCode. What do you think?

taras-yemets · 2022-12-21T16:31:52Z

I assume that maybe a few users could be left on the old view until we remove the code in the following release. For them the experience would be subpar but also not horribly broken. And they can always enable CodeMirror to fix it and get a lot of other improvements like faster rendering

CodeMirror blob is enabled by default on dotcom and s2 instances and would likely be the default in the upcoming release. With that said, I think we can even not ship recency and hunks separators for the table code view (unless it adds too much code overhead in reused BlameDecoration component).

philipp-spiess · 2022-12-21T16:32:14Z

I think we should update the 'old' table blob view styling to look similar to the CodeMirror blob: the blame section seems a bit condensed, recency indicators overflow sibling lines, and hunks separators are not visible.

I fixed the line height at least, that way it doesn't feel broken anymore

Now that we have these awesome hunks separators I think it would be nice to highlight the line code content when the corresponding blame hunk is hovered (even if it's not the first line of the hunk) similar to JetBrains and VSCode. What do you think?

Not sure I understand (the GIF is very small 😢) but yeah the hover area feels weirder now. Let's do this as a follow-up though?

taras-yemets · 2022-12-21T16:37:56Z

Not sure I understand (the GIF is very small 😢) but yeah the hover area feels weirder now.

Screen.Recording.2022-12-21.at.18.24.33.mov

Let's do this as a follow-up though?

Makes sense 👍🏻

philipp-spiess · 2022-12-21T16:49:12Z

@taras-yemets Much better! This is now with the legacy blob view:

valerybugakov · 2022-12-22T04:27:40Z

client/web/src/repo/blob/BlameRecency.tsx

+// The values are sampled from the following function:
+//   y=0.005*1.7^x


valerybugakov

Tested locally. It looks awesome!

client/web/src/repo/blob/BlameRecency.tsx

client/web/src/repo/blame/useBlameHunks.ts

philipp-spiess · 2022-12-22T12:49:02Z

I noticed a stupid bug with the old blob view and the top spacer when blame is enabled 😢

philipp-spiess added the needs-design Design requests - add to 'design priorities' project, add a deadline, if possible.. label Nov 11, 2022

cla-bot bot added the cla-signed label Nov 11, 2022

philipp-spiess added the team/code-exploration Issues owned by the Code Exploration team label Nov 16, 2022

philipp-spiess mentioned this pull request Nov 23, 2022

[Accessibility]: move blame info from gutter into CodeMirror decorations to make them readable by screenreaders #44148

Merged

philipp-spiess force-pushed the ps/blame-recency branch from 103016e to 53e3fb4 Compare November 30, 2022 15:35

philipp-spiess added the blocked label Dec 1, 2022

philipp-spiess force-pushed the ps/blame-recency branch from 53e3fb4 to 40a0c6d Compare December 20, 2022 16:29

philipp-spiess removed needs-design Design requests - add to 'design priorities' project, add a deadline, if possible.. blocked labels Dec 21, 2022

philipp-spiess requested review from a team December 21, 2022 15:38

philipp-spiess self-assigned this Dec 21, 2022

philipp-spiess marked this pull request as ready for review December 21, 2022 15:38

philipp-spiess requested review from mrnugget and jhchabran December 21, 2022 15:38

philipp-spiess commented Dec 21, 2022

View reviewed changes

taras-yemets reviewed Dec 21, 2022

View reviewed changes

client/web/src/repo/blob/codemirror/blame-decorations.tsx Outdated Show resolved Hide resolved

taras-yemets reviewed Dec 21, 2022

View reviewed changes

taras-yemets approved these changes Dec 21, 2022

View reviewed changes

philipp-spiess added 14 commits December 21, 2022 17:56

WIP blame recency

ea13658

Add 1px offset on the end of the hunk

1e03133

Improve tooltip delay

64a09c1

Add exponential blame recency calculation and borders

9e6f820

Fix avatar underline

8c94af6

Cleanup

d495b8f

Add first commit date and pass theme property

194e23e

Fixes

4cdba05

Add changelog and more fixes

9fa19c9

Fix style lint

5164362

Cleanup some comments

17eba00

Fix typo

26f876e

Prettier

60a0b00

Fix line height for legacy implementaiton

9369ef7

philipp-spiess force-pushed the ps/blame-recency branch from ae06a82 to 9369ef7 Compare December 21, 2022 16:56

valerybugakov reviewed Dec 22, 2022

View reviewed changes

valerybugakov approved these changes Dec 22, 2022

View reviewed changes

client/web/src/repo/blob/BlameRecency.tsx Outdated Show resolved Hide resolved

client/web/src/repo/blame/useBlameHunks.ts Outdated Show resolved Hide resolved

client/web/src/repo/blame/useBlameHunks.ts Outdated Show resolved Hide resolved

philipp-spiess added 2 commits December 22, 2022 12:53

Adress PR feedback

72c9d4e

Only increase height when git blame view is rendered

203af6a

philipp-spiess added 3 commits December 22, 2022 14:05

Fix update issue by passing the first commit date properly

f0c4daf

Revert behavior for old blob view

a4e8851

Fix linter issue

8e8c772

philipp-spiess merged commit b99358f into main Dec 22, 2022

philipp-spiess deleted the ps/blame-recency branch December 22, 2022 14:55

philipp-spiess mentioned this pull request Dec 22, 2022

Fix FirstEverCommit argument order #45934

Merged

jhchabran mentioned this pull request Jan 16, 2023

streaming git blame: add users fields #46496

Closed

fkling mentioned this pull request Jan 18, 2024

blob: Fix text selection visibility over selected lines and line number alignment #59712

Merged

	// We can not access the light theme from the view props beacuse we need
	// We can not access the light theme from the view props because we need

		// The values are sampled from the following function:
		// y=0.005*1.7^x

Blame UI improvements #44287

Blame UI improvements #44287

Uh oh!

Conversation

philipp-spiess commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

App preview:

Uh oh!

sg-e2e-regression-test-bob commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taras-yemets left a comment

Choose a reason for hiding this comment

Uh oh!

taras-yemets commented Dec 21, 2022

Uh oh!

philipp-spiess commented Dec 21, 2022

Uh oh!

taras-yemets commented Dec 21, 2022

Uh oh!

philipp-spiess commented Dec 21, 2022

Uh oh!

taras-yemets commented Dec 21, 2022

Uh oh!

taras-yemets commented Dec 21, 2022

Uh oh!

philipp-spiess commented Dec 21, 2022

Uh oh!

taras-yemets commented Dec 21, 2022

Uh oh!

philipp-spiess commented Dec 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valerybugakov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philipp-spiess commented Dec 22, 2022

Uh oh!

Uh oh!

philipp-spiess commented Nov 11, 2022 •

edited

Loading

sg-e2e-regression-test-bob commented Nov 11, 2022 •

edited

Loading