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

client/web: Use perforce cid in tree URL#54051

Merged
indradhanush merged 3 commits into
mainfrom
ig/cl-tree
Jun 26, 2023
Merged

client/web: Use perforce cid in tree URL#54051
indradhanush merged 3 commits into
mainfrom
ig/cl-tree

Conversation

@indradhanush

Copy link
Copy Markdown
Contributor

Links to "Browse files @CID" will now directly go to the URL pointing to the CID instead of the Commit SHA.

Test plan

  • Tested manually
  • Builds should pass

@cla-bot cla-bot Bot added the cla-signed label Jun 23, 2023
@sourcegraph-bot

sourcegraph-bot commented Jun 23, 2023

Copy link
Copy Markdown
Contributor

📖 Storybook live preview

@indradhanush indradhanush requested review from a team and peterguy June 23, 2023 14:03
@indradhanush indradhanush self-assigned this Jun 23, 2023
@indradhanush indradhanush added the perforce-changelists Issues around Changelist Ids for Perforce depots label Jun 23, 2023
@indradhanush indradhanush marked this pull request as draft June 23, 2023 14:12
@indradhanush

Copy link
Copy Markdown
Contributor Author

Found another place where this needs to be updated. Adding that code. Please hold onto the reviews till then.

@sashaostrikov sashaostrikov 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.

LGTM! One import needs to be fixed :)

import { Timestamp } from '@sourcegraph/branded/src/components/Timestamp'
import { pluralize } from '@sourcegraph/common'
import { Button, ButtonGroup, Link, Icon, Code, screenReaderAnnounce, Tooltip } from '@sourcegraph/wildcard'
import { Button, ButtonGroup, Link, Icon, Code, screenReaderAnnounce, Tooltip, ErrorAlert } from '@sourcegraph/wildcard'

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.

I guess the linter will fail otherwise

Suggested change
import { Button, ButtonGroup, Link, Icon, Code, screenReaderAnnounce, Tooltip, ErrorAlert } from '@sourcegraph/wildcard'
import { Button, ButtonGroup, ErrorAlert, Link, Icon, Code, screenReaderAnnounce, Tooltip } from '@sourcegraph/wildcard'

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.

Hmm, vscode added that and the linter didn't fail. 🤔
Is that enforced usually?

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.

Maybe it's not enforced and the linter won't fail, we just usually them in an alphabetic order.

Ah, the linter would've failed if the import statements themselves (e.g. import lines) were out of alphabetic order.

Sorry for confusion!

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.

No worries - I prefer them in the alphabetical order too. Thanks for the call out!

Comment thread client/web/src/repo/commits/GitCommitNode.tsx Outdated
- Also make a utility func to check the experimental flag

And address reviews comments:
- Remove margin
- Fix import
@indradhanush indradhanush marked this pull request as ready for review June 23, 2023 15:03

@sashaostrikov sashaostrikov 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.

LGTM!

@indradhanush indradhanush merged commit 0116fb6 into main Jun 26, 2023
@indradhanush indradhanush deleted the ig/cl-tree branch June 26, 2023 05:49
github-actions Bot pushed a commit that referenced this pull request Jun 26, 2023
Links to "Browse files @CID" will now directly go to the URL pointing to
the CID instead of the Commit SHA.

(cherry picked from commit 0116fb6)
keegancsmith pushed a commit that referenced this pull request Jun 27, 2023
Links to "Browse files @CID" will now directly go to the URL
pointing to the CID instead of the Commit SHA.

## Test plan

- Tested manually
- Builds should pass

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->
 <br> Backport 0116fb6 from #54051

Co-authored-by: Indradhanush Gupta <indradhanush.gupta@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed perforce-changelists Issues around Changelist Ids for Perforce depots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants