Add commit message length configuration border #1048

jamessspanggg · 2020-02-03T07:21:05Z

GitHub's maximum length of commit message's subject line
and body is defaulted at 50 and 72 characters respectively.

Commits panel does not show the indication of the maximum
lengths.

Let's add a dotted lines for commit message bodies at 72
characters and subject lines at 50 characters, these
lines indicate whether a commit message's subject
line or body exceed GitHub's maximum length.

damithc · 2020-02-03T07:28:42Z

Let's add a dashed border for commit message bodies at 72
characters as well as scrollable boxes for subject line that
is fixed at 50 characters.

For this, I meant just draw a box behind the text so that we can see if the subject is longer than 50 chars. A scrollable box will require users to scroll, which is unnecessary work.

jamessspanggg · 2020-02-03T07:34:06Z

Ok noted prof.

jamessspanggg · 2020-02-03T08:20:33Z

@damithc since the commit message subject line is limited to 50 characters, should we change the font to monospace (currently it is "Titillium Web",sans-serif;) ? So that each font takes equal amount of space and it is easier to get the length of 50 characters.

Changing the font however, might introduce inconsistencies in font between commits panel and authorship panel.

damithc · 2020-02-03T09:54:54Z

@damithc since the commit message subject line is limited to 50 characters, should we change the font to monospace (currently it is "Titillium Web",sans-serif;) ? So that each font takes equal amount of space and it is easier to get the length of 50 characters.

Changing the font however, might introduce inconsistencies in font between commits panel and authorship panel.

I think we should use the same monospace font for code, github commit message (subject and body) as all are code-like. Anyway, try it and post some screenshots to see how it looks.

jamessspanggg · 2020-02-03T13:20:35Z

I think we should use the same monospace font for code, github commit message (subject and body) as all are code-like. Anyway, try it and post some screenshots to see how it looks.

I have tried two ways of showing the 50 character limitation:

With the box that wraps the text at 50 characters.
Cons: looks slightly odd.

Using the ellipsis icon from the 50th character onwards:
Cons: require user to hover over the link to see full commit subject line

damithc · 2020-02-03T14:29:58Z

We don't want to hide anything; just want to shame the ones that violate the limit. What if you show a vertical white dotted line after 50 chars. Subtle but makes the point.

jamessspanggg · 2020-02-03T16:13:23Z

@damithc switched both limit lines to be dotted. Are the dotted lines in the commit message title too subtle?

damithc · 2020-02-04T01:24:12Z

@damithc switched both limit lines to be dotted. Are the dotted lines in the commit message title too subtle?

Yes. Use black, which is also consistent with what we do in the message body?

jamessspanggg · 2020-02-04T02:32:22Z

Yes. Use black, which is also consistent with what we do in the message body?

…e-line

jamessspanggg · 2020-02-04T08:42:13Z

frontend/src/static/css/style.scss

@@ -321,7 +333,7 @@ header {
  }

  &--button {
-    color: mui-color('grey', '700');
+    color: mui-color('grey');


Modified to standardise with the icon colors in v_authorship

0blivious

See the message below, the dotted line appear in the middle of the PR number. For the commit title length, I feel we should not count the PR number as it is not input by the user.

damithc · 2020-02-05T14:26:18Z

See the message below, the dotted line appear in the middle of the PR number. For the commit title length, I feel we should not count the PR number as it is not input by the user.

PR number is part of the subject line, added by the user. It's just a project convention, not automatic.

jamessspanggg · 2020-02-05T15:38:01Z

Yep placing the PR number is a choice, not automatic.

frontend/src/static/css/style.scss

Tejas2805

All looks good. Can you just add blank lines between the styles ?

# Conflicts: # frontend/src/tabs/zoom.pug

jamessspanggg · 2020-02-23T01:34:45Z

Perhaps the underline can be solid and the dashed line for the 72 characters can use lighter shade of grey? Do wait for @damithc 's input too.

Ok noted. Updated the previous comment with the UI.

damithc · 2020-02-23T08:17:12Z

Perhaps the underline can be solid and the dashed line for the 72 characters can use lighter shade of grey? Do wait for @damithc 's input too.

hmm... After seeing both, I kind of prefer vertical lines; while it may not be nice to look at, a vertical line is a good visual cue to indicate 'it should end here'. Also for consistency, we should make make the two as similar as possible, given both have the same purpose. If both look the same, that should also alleviate the worry that users will mistake it as a glitch. What do you think?

damithc · 2020-02-23T08:18:31Z

BTW, I believe there is an another ongoing PR that is trying to tweak the page background color, which has a bearing on the colors we choose for this feature.

jamessspanggg · 2020-02-23T08:37:29Z

BTW, I believe there is an another ongoing PR that is trying to tweak the page background color, which has a bearing on the colors we choose for this feature.

I believe it's from this PR #1080, which modifies the code and commits panel to be white.

hmm... After seeing both, I kind of prefer vertical lines; while it may not be nice to look at, a vertical line is a good visual cue to indicate 'it should end here'. Also for consistency, we should make make the two as similar as possible, given both have the same purpose.

I agree that we should use vertical lines for both. Would grey vertical dashed lines be alright? Grey would look ok on white backgrounds, as shown in the commit message body.

eugenepeh · 2020-02-23T08:53:55Z

hmm... After seeing both, I kind of prefer vertical lines; while it may not be nice to look at, a vertical line is a good visual cue to indicate 'it should end here'. Also for consistency, we should make make the two as similar as possible, given both have the same purpose. If both look the same, that should also alleviate the worry that users will mistake it as a glitch. What do you think?

My concern on vertical line for the title is more on the inconsistency of presence (when the title is shorter than 50 chars, it won't appear), which is why it kind of give off the "glitch" look to me.

Though I agree that it may be easier to identify the 50 chars cut off point, but I personally feel that underline would probably look more professional because it looks similar to how people are writing beyond the allocated space on forms and etc.

damithc · 2020-02-23T11:47:33Z

Though I agree that it may be easier to identify the 50 chars cut off point, but I personally feel that underline would probably look more professional because it looks similar to how people are writing beyond the allocated space on forms and etc.

I see what you mean. The form field analogy didn't occur to me because that behavior never happens in an online form, and in Web pages, underlines usually mean other things (such as links or tooltips). Let's stick with vertical lines for now. It will look a bit odd, but probably we can make it subtle enough to not to irritate the viewer. Or it may be OK for it to be irritating to the viewer in this instance because we are exposing an incorrect behavior of the commit author. The line will not even appear if the authors stuck to the standard. @jamessspanggg let's make both lines same thickness and same style, and if possible, same shade of grey.

jinyao-lee · 2020-02-23T11:49:18Z

What about this alternative instead of the dotted line:

Any commit subject that exceeds 50 char will have a ⚠️ sign or similar that states commit message exceeds 50 char upon being hovered?

damithc · 2020-02-23T12:08:08Z

Any commit subject that exceeds 50 char will have a ⚠️ sign or similar that states commit message exceeds 50 char upon being hovered?

I feel that's beyond RepoSense's role. We should only provide a basis for the user to make their own judgement, without providing our own judgement.

Let's go with the current approach, dogfood for a while, and refine later if it doesn't work.

# Conflicts: # docs/images/commits-panel.png # docs/images/opening-commits-panel.gif # frontend/src/static/css/style.scss # frontend/src/tabs/zoom.pug

# Conflicts: # frontend/src/static/css/style.scss

Tejas2805 · 2020-03-03T06:23:58Z

@fzdy1914 Can merge this first? Another PR by @jamessspanggg on hold due to this.

fzdy1914 · 2020-03-04T14:01:49Z

@jamessspanggg CI is failing, can you kindly recheck?

* 'master' of https://github.com/reposense/RepoSense: [reposense#1047] v_summary: simplify toDisplay user logic (reposense#1051) [reposense#658] Modify checkstyle configuration (reposense#1094) Add commit message length configuration border (reposense#1048) [reposense#1061] build.gradle: remove unused dependency (reposense#1095) [reposense#1044] Update date hashes on reset date range (reposense#1068)

Add dashed border at 72 characters

7dad836

jamessspanggg mentioned this pull request Feb 4, 2020

[#1005,#1010] Code Panel: match font with GitHub and break lines when overflow #1008

Merged

5 tasks

jamessspanggg added 4 commits February 4, 2020 15:21

Add border for commit subject line and standardise UI

429b82f

Merge branch 'master' into add-commit-message-line

7b1b6ed

Remove trailing white space

ca1a760

Merge remote-tracking branch 'upstream/master' into add-commit-messag…

a478859

…e-line

jamessspanggg marked this pull request as ready for review February 4, 2020 08:41

jamessspanggg commented Feb 4, 2020

View reviewed changes

jamessspanggg requested a review from a team February 4, 2020 08:42

jamessspanggg added the s.ToReview label Feb 4, 2020

0blivious reviewed Feb 5, 2020

View reviewed changes

jamessspanggg requested review from 0blivious and a team February 6, 2020 15:29

0blivious suggested changes Feb 12, 2020

View reviewed changes

frontend/src/static/css/style.scss Outdated Show resolved Hide resolved

frontend/src/static/css/style.scss Outdated Show resolved Hide resolved

Tejas2805 requested changes Feb 12, 2020

View reviewed changes

jamessspanggg added 2 commits February 12, 2020 18:47

Add blank lines between css classes

562e999

Merge branch 'master' into add-commit-message-line

8af16aa

# Conflicts: # frontend/src/tabs/zoom.pug

jamessspanggg requested review from Tejas2805 and 0blivious February 12, 2020 10:58

jamessspanggg requested a review from damithc February 23, 2020 07:38

jamessspanggg added 2 commits February 23, 2020 20:11

Standardise to be grey dashed line

15a9b69

Merge branch 'master' into add-commit-message-line

d07ef96

jamessspanggg requested review from damithc and removed request for damithc February 23, 2020 12:19

damithc approved these changes Feb 23, 2020

View reviewed changes

jamessspanggg requested a review from eugenepeh February 23, 2020 13:33

jamessspanggg added 4 commits February 25, 2020 17:44

Merge branch 'master' into add-commit-message-line

1174ed1

# Conflicts: # docs/images/commits-panel.png # docs/images/opening-commits-panel.gif # frontend/src/static/css/style.scss # frontend/src/tabs/zoom.pug

Update ug diagrams

0f2f661

Update ug zoom tab gif

3f12e3b

Merge branch 'master' into add-commit-message-line

5a86723

# Conflicts: # frontend/src/static/css/style.scss

jamessspanggg requested review from eugenepeh and removed request for eugenepeh February 29, 2020 07:45

eugenepeh approved these changes Feb 29, 2020

View reviewed changes

jamessspanggg added s.ToMerge and removed s.FinalReview labels Feb 29, 2020

Merge branch 'master' into add-commit-message-line

0e9c7af

Merge branch 'master' into add-commit-message-line

8d05100

fzdy1914 merged commit 9725a25 into reposense:master Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commit message length configuration border #1048

Add commit message length configuration border #1048

jamessspanggg commented Feb 3, 2020 •

edited

Loading

damithc commented Feb 3, 2020

jamessspanggg commented Feb 3, 2020

jamessspanggg commented Feb 3, 2020

damithc commented Feb 3, 2020

jamessspanggg commented Feb 3, 2020 •

edited

Loading

damithc commented Feb 3, 2020 •

edited

Loading

jamessspanggg commented Feb 3, 2020 •

edited

Loading

damithc commented Feb 4, 2020

jamessspanggg commented Feb 4, 2020

jamessspanggg Feb 4, 2020

0blivious left a comment •

edited

Loading

damithc commented Feb 5, 2020

jamessspanggg commented Feb 5, 2020

Tejas2805 left a comment

jamessspanggg commented Feb 23, 2020

damithc commented Feb 23, 2020

damithc commented Feb 23, 2020

jamessspanggg commented Feb 23, 2020

eugenepeh commented Feb 23, 2020 •

edited

Loading

damithc commented Feb 23, 2020

jinyao-lee commented Feb 23, 2020 •

edited

Loading

damithc commented Feb 23, 2020

Tejas2805 commented Mar 3, 2020

fzdy1914 commented Mar 4, 2020

Add commit message length configuration border #1048

Add commit message length configuration border #1048

Conversation

jamessspanggg commented Feb 3, 2020 • edited Loading

damithc commented Feb 3, 2020

jamessspanggg commented Feb 3, 2020

jamessspanggg commented Feb 3, 2020

damithc commented Feb 3, 2020

jamessspanggg commented Feb 3, 2020 • edited Loading

damithc commented Feb 3, 2020 • edited Loading

jamessspanggg commented Feb 3, 2020 • edited Loading

damithc commented Feb 4, 2020

jamessspanggg commented Feb 4, 2020

jamessspanggg Feb 4, 2020

Choose a reason for hiding this comment

0blivious left a comment • edited Loading

Choose a reason for hiding this comment

damithc commented Feb 5, 2020

jamessspanggg commented Feb 5, 2020

Tejas2805 left a comment

Choose a reason for hiding this comment

jamessspanggg commented Feb 23, 2020

damithc commented Feb 23, 2020

damithc commented Feb 23, 2020

jamessspanggg commented Feb 23, 2020

eugenepeh commented Feb 23, 2020 • edited Loading

damithc commented Feb 23, 2020

jinyao-lee commented Feb 23, 2020 • edited Loading

damithc commented Feb 23, 2020

Tejas2805 commented Mar 3, 2020

fzdy1914 commented Mar 4, 2020

jamessspanggg commented Feb 3, 2020 •

edited

Loading

jamessspanggg commented Feb 3, 2020 •

edited

Loading

damithc commented Feb 3, 2020 •

edited

Loading

jamessspanggg commented Feb 3, 2020 •

edited

Loading

0blivious left a comment •

edited

Loading

eugenepeh commented Feb 23, 2020 •

edited

Loading

jinyao-lee commented Feb 23, 2020 •

edited

Loading