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.

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

pr-checks-header - New feature #7160

Closed
wants to merge 3 commits into from
Closed

pr-checks-header - New feature #7160

wants to merge 3 commits into from

Conversation

cheap-glitch
Copy link
Member

@cheap-glitch cheap-glitch commented Dec 15, 2023

Resolves #4086

Test URLs

Additional tests:

  • Edit the title. The CI link should be added again.
  • Edit a PR in a new tab and push a new commit to the branch. The CI link should be updated.
  • Wait for the checks to be resolved. The CI link should be updated.

Screenshots

Conversation tab

image

Commits tab

image

Files tab

image

Comment on lines +11 to +16
/* Ensure the CI details dropdown always opens to the right */
.gh-header-title .commit-build-statuses .dropdown-menu {
left: 100%;
right: auto;
margin: 24px 0 0 8px;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Opening the dropdown on the right seems like the safer option, but it will still get clipped if the window is narrow and PR title ends all the way to the right.

Idk if GitHub has "automatic" dropdowns that open on the correct side, I haven't found anything in their style guide.

Copy link
Member

@fregante fregante Dec 15, 2023

Choose a reason for hiding this comment

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

Maybe then it should be positioned elsewhere, not next to the title. See #4086 (comment)

Comment on lines +28 to +30
/*

Test URLs:
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems useless to have to duplicate test URLs in the feature CSS file, is this intended?

Copy link
Member

@fregante fregante Dec 15, 2023

Choose a reason for hiding this comment

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

This is not enforced at the moment, the tester currently specifies *.tsx only, so they should be dropped.


$('#partial-discussion-header')!.classList.add('rgh-pr-ci-link-added');

const ciLinkCommitSha = /[a-f\d]{40}/.exec(iconWrapper.dataset.url!)![0];
Copy link
Member Author

Choose a reason for hiding this comment

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

Use the commit OID to check if the CI link needs updating. Sometimes it's an URL parameter, sometimes it's in the pathname.

include: [
pageDetect.isPR,
],
init: initPR,
Copy link
Member

Choose a reason for hiding this comment

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

There's zero overlap here, worth extracting ci-link-pr? or pr-checks-header?

const ciLinkIsInDom = pageDetect.isPRConversation() || pageDetect.isPRCommitList();

if (ciLinkIsInDom) {
observe(':is(#discussion_bucket, #commits_bucket) .commit-build-statuses summary', addPRIcon, {signal});
Copy link
Member

Choose a reason for hiding this comment

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

Note that this will trigger for each and every checked-commit on the page. You might just want to use awaitDomReady and lastElement instead, even if it's slightly longer.

However I'd probably just skip this and always use fetchAndAddPrIcon for simplicity.

Comment on lines +28 to +30
/*

Test URLs:
Copy link
Member

@fregante fregante Dec 15, 2023

Choose a reason for hiding this comment

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

This is not enforced at the moment, the tester currently specifies *.tsx only, so they should be dropped.

Comment on lines +11 to +16
/* Ensure the CI details dropdown always opens to the right */
.gh-header-title .commit-build-statuses .dropdown-menu {
left: 100%;
right: auto;
margin: 24px 0 0 8px;
}
Copy link
Member

@fregante fregante Dec 15, 2023

Choose a reason for hiding this comment

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

Maybe then it should be positioned elsewhere, not next to the title. See #4086 (comment)

@fregante
Copy link
Member

@fregante fregante changed the title Extend ci-link to PR title pr-checks-header - New feature Jan 10, 2024
@fregante fregante closed this Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Extend ci-link to PR title
2 participants