Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

blob: make selection-driven nav default#48066

Merged
taras-yemets merged 28 commits into
mainfrom
taras-yemets/cm-blob-cleanup
Feb 28, 2023
Merged

blob: make selection-driven nav default#48066
taras-yemets merged 28 commits into
mainfrom
taras-yemets/cm-blob-cleanup

Conversation

@taras-yemets

@taras-yemets taras-yemets commented Feb 22, 2023

Copy link
Copy Markdown
Contributor
  1. Makes selection-driven code navigation single code navigation option.
  2. Removes link-driven code navigation.
  3. Removes hover-only code navigation.

Selection-driven navigation has been dogfooded on dotcom and s2 instances for a while, so significant issues should have been addressed already. It's relatively safe to remove link-driven (not widely used) and hover-only code navigation implementation and leave selection-driven as the only way to navigate the blob view.

It was suggested initially to leave a fallback option, but hover-only navigation was not used internally for a while and we probably can't be sure if it is still fully functional given the amount of recent CodeMirror blob config changes.

Feedback is highly appreciated!

Screen.Recording.2023-02-23.at.17.34.10.mov

Test plan

  • CI passes
  • Tested manually (video attached)

@cla-bot cla-bot Bot added the cla-signed label Feb 22, 2023
@sg-e2e-regression-test-bob

sg-e2e-regression-test-bob commented Feb 22, 2023

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) -0.10% (-14.79 kb) 🔽 -0.13% (-14.79 kb) 🔽 0.00% (0)

Look at the Statoscope report for a full comparison between the commits 2fcaa89 and 397fe92 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

@taras-yemets taras-yemets requested review from a team, fkling and olafurpg February 23, 2023 15:47
@taras-yemets taras-yemets marked this pull request as ready for review February 23, 2023 15:47
@sqs

sqs commented Feb 26, 2023

Copy link
Copy Markdown
Member

This would be great. It would remove a lot of unused code paths!

@sqs

sqs commented Feb 26, 2023

Copy link
Copy Markdown
Member

I rebased the PR https://github.com/sourcegraph/sourcegraph/pull/46059 (remove the extension API) on top of this. That removal significantly benefits from this change (thank you!). Do you plan to merge this PR in the next few days (or at least in the next 1-2 weeks)? Just wondering so I can plan to complete that PR by the next release.

@taras-yemets

Copy link
Copy Markdown
Contributor Author

Great, @sqs!
I'm going to merge it in the next few days.

Comment thread schema/settings.schema.json Outdated
// URL is updated
await driver.assertWindowLocation(`${filePaths['test.ts']}?L1-3`)
})

@taras-yemets taras-yemets Feb 27, 2023

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.

Removing tests for not supported behavior.
With the current selection-driven nav implementaion clicking (shift-clicking) only line numbers select line (ranges).
Clicking on the line content has no effect. Shift-clicking lines selects lines content.

Screen.Recording.2023-02-27.at.14.41.49.mov

@philipp-spiess philipp-spiess left a comment

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.

Don't forget to add a changelog but this is 🕶️

Comment thread client/web/src/repo/blob/CodeMirrorBlob.tsx
@taras-yemets taras-yemets merged commit 578686f into main Feb 28, 2023
@taras-yemets taras-yemets deleted the taras-yemets/cm-blob-cleanup branch February 28, 2023 11:00
sqs referenced this pull request Mar 6, 2023
In the web app, the new selection-driven code nav UI
(https://github.com/sourcegraph/sourcegraph/pull/44698
https://github.com/sourcegraph/sourcegraph/pull/48066) does not use
this.

The browser extension's implementation is separate from the web app's in
implementation and backstory, but it is also removed because it presents
the same future UX problems and is inconsistent with the direction we're
taking for the web app.

## Test plan

Ensure code navigation in the remaining selection-driven mode still
works.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants