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 small space between commit summary and description #3922

Merged
merged 2 commits into from Feb 12, 2018

Conversation

Projects
None yet
3 participants
@willnode
Contributor

willnode commented Feb 3, 2018

I noticed that the commit list has been redesigned. I think it can be better a bit.

This commit brings down commit description by 3px while keeping commit summary in place.

Preview:

screenshot 28

Close up difference:

screenshot 27

Add small space between commit summary and description
This commit brings down commit description by 3px while keeping commit summary in place.
@shiftkey

This comment has been minimized.

Member

shiftkey commented Feb 3, 2018

@willnode thanks for the contribution, and thanks for the detailed visual comparison.

I'll let @desktop/design chime in about this when they have a moment.

@shiftkey shiftkey added this to the 1.1 milestone Feb 7, 2018

@donokuda

This comment has been minimized.

Contributor

donokuda commented Feb 7, 2018

Hi @willnode, thanks for the pull request! Can you explain the reason for bumping down the commit description?

One other thing to note, you'll also need to increase the row height of the list item here (probably by the amount you changed):

const RowHeight = 48

This is because the height itself is fixed and the vertical padding becomes uneven as a result (the bottom padding as less space):

@shiftkey

Holding pending @donokuda's questions and comments.

@willnode

This comment has been minimized.

Contributor

willnode commented Feb 8, 2018

@donokuda The original design is already looking good. But whenever I see a commit message have difference of only 1 pixel below the profile picture (e.g. emoji or g in commit summary), it always come to my mind that I want to bump it down a little bit.

I wasn't knew where to change the row height so thanks for pointing that. I can make it bigger by 2px, but I'm waiting for feedback.

Updated comparison: (original; PR at this state; after resized by +2px)

githubdemo

@donokuda

This comment has been minimized.

Contributor

donokuda commented Feb 8, 2018

@willnode Thanks for the clarification! 🙇

I totally agree and this looks like a good improvement. When you push the changes I suggested above, feel free to request my review and I can jump in on it. (Bonus points for attaching a screenshot like how you've done earlier 📸 )

Enlarge commit RowHeight
This commit enlarges commit RowHeight by 2px while keeping commit summary stay in place.
@willnode

This comment has been minimized.

Contributor

willnode commented Feb 8, 2018

@donokuda All set!

And yeah, I done the whole thing with DevTool open. So I just have to edit the stylesheet and not having to clone and rebuild whole thing. The rest is just some magic with mspaint and powerpnt 🎨.

screenshot 54

Dismissing in favor of my own review since it's UI related

@donokuda

@donokuda donokuda merged commit 0f38a95 into desktop:master Feb 12, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@willnode willnode deleted the willnode:patch-2 branch Feb 14, 2018

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