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

Add behind count to update-pr-from-base-branch #6124

Closed
wants to merge 6 commits into from
Closed

Conversation

yakov116
Copy link
Member

@yakov116 yakov116 commented Nov 1, 2022

Closes #3863

Test URLs

refined-github/sandbox#53

Screenshot

image

} else {
position.append(' ', (
<span className="status-meta d-inline-block">
{select('.head-ref')!.cloneNode(true)} is {pluralize(prInfo.headRef.compare.behindBy, '$$ commit', '$$ commits')} behind {select('.base-ref')!.cloneNode(true)}
Copy link
Member

Choose a reason for hiding this comment

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

The head is available just a few words before this node, we shouldn't repeat it. If anything we can replace/reword it.

Before doing further work, let's see how else we can word it or what information we actually want here.

Copy link
Member Author

@yakov116 yakov116 Nov 1, 2022

Choose a reason for hiding this comment

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

What about
image

We can just append the DOM and GitHub will load it. (We just need the base commit and head commit)

<batch-deferred-content data-url="/refined-github/refined-github/ref-ahead-behind">
    <input type="hidden" name="base_commit" value="43ef4e52317f28ea1fbeb95f624fd4ca0b4f06ad" data-targets="batch-deferred-content.inputs" autocomplete="off">
    <input type="hidden" name="head_commit" value="f1fe231d9319f9efc8af33017e43ef029edfa346" data-targets="batch-deferred-content.inputs" autocomplete="off">

  
  <svg style="box-sizing: content-box; color: var(--color-icon-primary);" width="32" height="32" viewBox="0 0 16 16" fill="none" data-view-component="true" class="anim-rotate">
  <circle cx="8" cy="8" r="7" stroke="currentColor" stroke-opacity="0.25" stroke-width="2" vector-effect="non-scaling-stroke"></circle>
  <path d="M15 8a7.002 7.002 0 00-7-7" stroke="currentColor" stroke-width="2" stroke-linecap="round" vector-effect="non-scaling-stroke"></path>
</svg>

Don't like it but just throwing in the idea

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 also the problem that this feature only appears when the native button doesn't (or at least that's the intention), so we probably shouldn't bundle this information with this feature.

What if we just add it to the PR header next to the branches? Like:

(Open) main <- feature (-5)

The number of "+" commits is already visible on the PR Commits tab. The number could show further information on in a tooltip.

I'm not totally convinced by this however as the commit could be useful to be copied. I'm open to pushing a minimal version and then iterate on it as we start to rely on this piece of information.

I'd also perhaps just append a simple version of this to both "Update branch" buttons (ours and the native one), and make it easier. Those areas at the bottom have plenty of space

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'd also perhaps just append a simple version of this to both "Update branch" buttons (ours and the native one), and make it easier. Those areas at the bottom have plenty of space

Lost you on this line

Copy link
Member Author

@yakov116 yakov116 Nov 1, 2022

Choose a reason for hiding this comment

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

What if we just add it to the PR header next to the branches? Like:

(Open) main <- feature (-5)

Like I as going to try to do this, where are we putting the base commit link?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should it be part of clean-conversation-headers

Copy link
Member

Choose a reason for hiding this comment

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

Lost you on this line

Native

Screen Shot 5

Ours

Screen Shot 6

Change requested

Show "base commit" near both of these. This feature isn't necessarily related to any other. It's not cleaning and it's not necessarily updating the branch.

Copy link
Member

@fregante fregante Nov 2, 2022

Choose a reason for hiding this comment

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

What if we just add it to the PR header next to the branches? Like:

(Open) main <- feature (-5)

Like I as going to try to do this, where are we putting the base commit link?

👇

The number could show further information in a tooltip.

With "further information" being the base commit. I followed that with a comment regarding the need to copy or see this value.

Copy link
Member Author

Choose a reason for hiding this comment

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

In other words we should change it to?

image

source/features/update-pr-from-base-branch.tsx Outdated Show resolved Hide resolved
source/features/update-pr-from-base-branch.tsx Outdated Show resolved Hide resolved
@yakov116 yakov116 closed this Nov 6, 2022
@yakov116 yakov116 deleted the base_behind branch November 6, 2022 16:51
@yakov116 yakov116 mentioned this pull request Apr 17, 2023
1 task
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.

Show base commit in PRs (and not just the base branch)
2 participants