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.

Sign up for GitHub

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

Jump to bottom

Fixes issue autolinks rendering when autolinks enabled is false #3924

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fixes issue autolinks rendering when autolinks enabled is false #3924

wants to merge 2 commits into from
+11 −1

Conversation

nzaytsev
Copy link
Contributor

@nzaytsev nzaytsev commented Jan 14, 2025 •
edited
Loading

Description

Screen.Recording.2025-01-14.at.09.37.59.mov

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

@nzaytsev nzaytsev linked an issue Jan 14, 2025 that may be closed by this pull request
Inspect & Graph Details: issue autolinks rendering when autolinks enabled is false #3841
Open
@nzaytsev nzaytsev requested review from eamodio and d13 January 14, 2025 02:42
@nzaytsev nzaytsev force-pushed the bugs/3841-autolinks-view-ignores-config branch from 50972cf to 9e58473 Compare January 16, 2025 06:13
@d13
Copy link
Member

d13 commented Jan 16, 2025

Hey @nzaytsev, while reviewing your PR, I'd suggest the following code changes:

👉 Still getting pull request data for autolinks

Although we're not displaying autolinks in the UI, we're still getting the pull request data for the autolinks and sending it to the webview.

You can also review and apply these suggestions locally on your machine.

Learn more about GitKraken Code Suggest

Code Suggest liberates your code reviews from GitHub's restrictive, comment-only feedback style. As simple as suggesting changes in a Google-doc, provide real code suggestions from where you code, e.g. your IDE, and on anything in your project — not just on the lines of code changed in the PR.

Join your team on GitKraken to speed up PR review.

d13
d13 requested changes Jan 16, 2025
View reviewed changes
Copy link
Member

@d13 d13 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please see comment in code suggestion: #3924 (comment)

@nzaytsev nzaytsev force-pushed the bugs/3841-autolinks-view-ignores-config branch from cd053da to 93ae6b4 Compare January 17, 2025 03:47
@nzaytsev
Copy link
Contributor Author

@d13 - fixed

@nzaytsev nzaytsev requested a review from d13 January 17, 2025 03:47
d13
d13 approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@d13 d13 left a comment

Choose a reason for hiding this comment

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

Some nits, otherwise looks good

src/webviews/apps/commitDetails/components/gl-commit-details.ts
Comment on lines 206 to +209
if (this.isUncommitted) return undefined;
if (!this.state?.autolinksEnabled) {
return nothing;
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
if (this.isUncommitted) return undefined;
if (!this.state?.autolinksEnabled) {
return nothing;
}
if (this.isUncommitted || !this.state?.autolinksEnabled) return undefined;

src/webviews/commitDetails/commitDetailsWebview.ts
Comment on lines +1242 to +1244
autolinksEnabled: configuration.get('views.commitDetails.autolinks.enabled') ?? false,
pullRequest:
configuration.get('views.commitDetails.autolinks.enabled') && current.pullRequest != null
Copy link
Member

Choose a reason for hiding this comment

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

Nit: consolidated the two configuration lookups

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d13 d13 d13 approved these changes

@eamodio eamodio Awaiting requested review from eamodio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inspect & Graph Details: issue autolinks rendering when autolinks enabled is false
2 participants
@nzaytsev @d13