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

Distinctly identify commits that refer pull request #1794

Closed
wants to merge 3 commits into from

Conversation

dakshshah96
Copy link

@dakshshah96 dakshshah96 commented Feb 15, 2019

Fixes #1579

@fregante
Copy link
Member

Can you post a URL where this would appear?

@dakshshah96
Copy link
Author

dakshshah96 commented Feb 16, 2019

You can take a look at this issue: #226

@fregante
Copy link
Member

fregante commented Feb 16, 2019

Bug

This also affects regular references (before/after this PR):

issue reference

Note

It looks nice, but I don't know if it fixes the issue discussed:

Confusing, looks like the pull request commits and the commit that references this pull request are in the same branch.

commit reference

I wonder if they should be taken completely off the line (margin-left: 100px):

offset

Also perhaps that blue makes it look more important on the timeline, rather than less important. Maybe we should only use margin-left

cc @sompylasar

@dakshshah96
Copy link
Author

I'll try fixing the bug now.

I agree that the blue does make it look more important on the timeline. Only using margin-left seems like a good solution.

@sompylasar
Copy link
Contributor

@bfred-it

I agree with blue vs margin : more-important vs same-importance.

Also some time later after making these styles I noticed that GitHub changed their markup and it became harder-or-impossible-with-CSS-only to determine the conditions for style application.

So reliability of this code has to be tested on a variation of scenarios (the community should definitely have more of them than me alone).

@jerone
Copy link
Contributor

jerone commented Feb 22, 2019

I see the vertical line as a timeline, this can contain commits, but also comments and also references. The reference contains a description and the commit where reference happened. My suggestion would be to combine them by removing the commit icon. Now it's just one reference.

First one has the commit icon removed:
image

The same is already done when referencing PR:
image

CSS:

.discussion-item-header[id^='ref-commit-'] + .discussion-item-body .timeline-commits .commit .commit-icon {
	visibility: hidden;
}

@fregante
Copy link
Member

fregante commented Feb 23, 2019

My suggestion would be to combine them by removing the commit icon

Yo, that's so smart and obvious that GitHub should pick it up immediately.

@lukehefson plz


Combining them is a little harder since they are technically different commits, you're potentially hiding useful information... unless GitHub compares them and groups them instead of completely hiding them; i.e. "2 similar commits" like for collapsed comments: hayaku/hayaku#303 (comment)

@jerone
Copy link
Contributor

jerone commented Feb 23, 2019

Combining them is a little harder

Sorry, I didn't meant to combine them to hide. More semantically combine them by removing the icon.

Is there an example where multiple commits are in one reference? I was thinking, maybe we can indent the commit icon only.

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Let's update this to only drop the commit icon and possibly drop the bigger "bookmark" icon to deemphasize it even more.

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.

None yet

4 participants