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

Improve reliability of list-prs-for-branch #6641

Merged
merged 2 commits into from
May 10, 2023
Merged

Conversation

@fregante fregante added the bug label May 10, 2023
@fregante
Copy link
Member Author

cc @DanielMiao1

@@ -55,7 +56,7 @@ void features.add(import.meta.url, {
include: [
pageDetect.isRepoTree,
pageDetect.isSingleFile,
pageDetect.isRepoCommitList,
isRepoCommitListRoot,
Copy link
Member Author

Choose a reason for hiding this comment

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

"List commits for files/folders" don't have a brach selector, so we don't add a default branch button either.

Compare:


await addAfterBranchSelector(link);
observe(branchSelectorParent, add, {signal});
Copy link
Member Author

Choose a reason for hiding this comment

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

Started using the observer, no more awkward code in addAfterBranchSelector

<a
className="btn ml-2 px-2 tooltipped tooltipped-ne"
className="btn px-2 tooltipped tooltipped-ne"
Copy link
Member Author

Choose a reason for hiding this comment

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

Now uses gap for spacing

@@ -248,3 +248,6 @@ export async function getError(apiResponse: JsonObject): Promise<RefinedGitHubAP
error.response = apiResponse;
return error;
}

// Export single API object as default
export * as default from './api.js';
Copy link
Member Author

Choose a reason for hiding this comment

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

Future lint:

  • s/import * as api/import api/

export function extractCurrentBranchFromBranchPicker(branchPicker: HTMLElement): string {
return branchPicker.title === 'Switch branches or tags'
? branchPicker.textContent!.trim() // Branch name is shown in full
: branchPicker.title; // Branch name was clipped, so they placed it in the title attribute
Copy link
Member Author

Choose a reason for hiding this comment

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

I had dropped this logic in #6629; it's what was breaking list-prs-for-branch on https://github.com/refined-github/sandbox/commits/6538-PR-BRANCH-2 specifically.

export function addAfterBranchSelector(branchSelectorParent: HTMLDetailsElement, sibling: HTMLElement): void {
const row = branchSelectorParent.closest('.position-relative')!;
row.classList.add('d-flex', 'flex-shrink-0', 'gap-2');
row.append(sibling);
Copy link
Member Author

Choose a reason for hiding this comment

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

Logic is much simpler nowadays because latest-tag-button is gone. This currently only applies to old non-React views:

  • repo home
  • repo commit list root

Gone is the long logic from #4236

@fregante fregante changed the title just fixing stuff Improve reliability of list-prs-for-branch May 10, 2023
@fregante fregante marked this pull request as ready for review May 10, 2023 21:28
@fregante fregante enabled auto-merge (squash) May 10, 2023 21:28
@fregante fregante merged commit 4865867 into main May 10, 2023
10 checks passed
@fregante fregante deleted the brand-selector-related-stuff branch May 10, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

The list-prs-for-branch button is mispositioned/vertically-misaligned/wrapped
1 participant