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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config setting for hard-wrapping commit message body #1223

Merged
merged 24 commits into from Dec 9, 2017

Conversation

Projects
None yet
7 participants
@kuychaco
Member

kuychaco commented Oct 19, 2017

Fixes #1148

This PR adds a config option for hard-wrapping the commit message body which defaults to true
settings_ ___src_test-repo

Open question:
This gives people an option to turn off hard-wrapping, but new users might still find themselves unpleasantly surprised because it's not initially apparent that the default is to insert new lines to wrap messages. To address this, we discussed doing some combination of the following when the config option is set to true:

  • change the commit button text to say "Format + Commit" (or something like it)
  • add a tooltip that says the message body will be hard-wrapped (doesn't show when committing from an expanded commit editor)

fullscreen_10_18_17__7_40_pm

Or maybe an asterisk will suffice? It's more discreet.

fullscreen_10_18_17__7_52_pm

Or perhaps an icon rather than an asterisk? Not sure what would be appropriate though.

Thoughts? 馃挱 I think I'm in favor of the asterisk or an icon... They're less descriptive, but more discreet and still draw enough attention to get the user to hover over the button and see the tooltip.

/cc @BinaryMuse @iolsen @smashwilson @damieng @donokuda

Todo:

  • consider stripping out comments using --cleanup=strip to git commit, which will remove the comments, and also strip leading and trailing empty lines, trailing whitespace, and collapse consecutive empty lines. There doesn鈥檛 seem to be an option for only removing comments.
@iolsen

This comment has been minimized.

iolsen commented Oct 19, 2017

I agree the Format + Commit is kind of gross. The asterisk is a slight improvement. I wonder if a paintbrush icon, or something similar to indicate styling, would work.

@kuychaco

This comment has been minimized.

Member

kuychaco commented Oct 19, 2017

Hmm yeah... really not a fan of changing the commit button text. How about an info icon in the commit message box, which I think @smashwilson originally suggested. It would only show up when relevant - that is, when the setting is enabled and when the user鈥檚 cursor is no longer on the first line and they are about to type out the commit message body. We discussed maybe showing the icon once the user hits 72 chars, but I think might be more jarring/distracting, potentially breaking the user鈥檚 attention while they鈥檙e right in the middle of typing out a thought.

git_ ___src_test-repo

WIP
@leroix

This comment has been minimized.

leroix commented Oct 19, 2017

Seeing this thread made me take a closer look at what happens for me in vim when git opens it for commit message editing.

screen shot 2017-10-19 at 12 46 25 pm

It's arguable that vim isn't the most beginner-friendly editor out there. However, it's also an interesting trade-off to consider whether you optimize for a beginner experience or a power-user experience. One way to get the best of both worlds might be to show the message once and allow the user to dismiss it.

@kuychaco

This comment has been minimized.

Member

kuychaco commented Oct 20, 2017

@leroix yeah, I like the one-time message idea. the question is then when/how do we show the message? Waiting until the user hits commit will be too late. Having a notification pop up when the user starts writing a commit body feels like it might be a bit jarring and intrusive, but maybe it's not so bad if it's just a one time thing that they dismiss...

@kuychaco

This comment has been minimized.

Member

kuychaco commented Oct 23, 2017

After some discussion with @nathansobo and @smashwilson in Slack we've settled on the following:

Iterating on this:

git_ ___src_test-repo

Replace the info icon with a custom hard-wrap icon that toggles hard-wrapping on and off when clicked, with a tooltip that says "Enable auto-wrap" or "Disable auto-wrap" depending on the state. Perhaps something like this:

icons

For discoverability purposes we can make the icon pop into view at the moment that the commit message spans multiple lines. Hopefully this is enough to grab the users attention. It also has the added benefit of not cluttering up the UI when the user is typing a single-line commit message and hard-wrapping doesn't apply.

UX questions/concerns:
( @donokuda could use your help here 馃檹 )

  • decide on the best icons to use, and make them (this is something I'm totally unqualified to do at the moment)
  • should we move the character currently located to the right of the commit button into the commit message box? Seems like the right place because it pertains to the message, but would that look funky?
@donokuda

This comment has been minimized.

Member

donokuda commented Nov 9, 2017

Hey! I'm so, so, sorry this dropped off my radar. 馃槥

I don't have much to add other than to validate what you all have iterate towards. I'm 馃挴 for adding an icon that toggles wrapping. We probably should still noodle on the specific icon used for toggling, but overall the experience feels like it's in the right direction.

Totally out of scope of for this pull request, but something to think in a future iteration, is being able to see how the commit message will be formatted with this setting. If I have hard line-wrapping enabled, would I be able to click on the "expanded commit message" button to the right to verify my formatting before I commit? (This might be useful if I'm writing long content in a bullet list)

should we move the character currently located to the right of the commit button into the commit message box? Seems like the right place because it pertains to the message, but would that look funky?

Yeah, I think it makes sense to put it in the commit message box, but I also don't have strong feelings either way (or the reasoning around its current location.) Might be worth just trying it out and see how it looks / feels.

@kuychaco

This comment has been minimized.

Member

kuychaco commented Nov 9, 2017

@donokuda thanks for your feedback and validation! This PR is pretty close to being merge-ready. Just need some fitting icons first (hoping you can help with this). For stand-in icons I ended up using no-newline and arrow-left, which probably aren't ideal.

hard-wrap

@kuychaco

This comment has been minimized.

Member

kuychaco commented Nov 9, 2017

Totally out of scope of for this pull request, but something to think in a future iteration, is being able to see how the commit message will be formatted with this setting. If I have hard line-wrapping enabled, would I be able to click on the "expanded commit message" button to the right to verify my formatting before I commit? (This might be useful if I'm writing long content in a bullet list)

These are good questions @donokuda. The short answer is "no". Hard-wrapping only happens when you commit. Expanding to the editor just carries the text over exactly as it is in the commit box.

Some context:

Currently, we're treating the commit box as the UI equivalent of using -m on the command line, plus the ability to add a commit message body. So it's for quick and simple commit messages, and we'll even format your message body for you by hard-wrapping it at 72 chars so that it looks nicer.

Committing from the expanded commit message editor is the equivalent to committing without -m. If you commit while an editor is open, we do absolutely no formatting of the commit message. What you see is what you get.

It's not clear what the best way is to convey all of this to the user.

@nathansobo

This comment has been minimized.

Contributor

nathansobo commented Nov 9, 2017

I'm of course cool with the arrow icons temporarily, but it would be great to get something custom eventually.

@kuychaco

This comment has been minimized.

Member

kuychaco commented Nov 14, 2017

Thankfully @donokuda has kindly agreed to make icons for us we he is able to. This should be the only thing left needed for this PR

@kuychaco kuychaco requested a review from BinaryMuse Nov 14, 2017

@donokuda

This comment has been minimized.

Member

donokuda commented Nov 14, 2017

Messed around with a couple of icons for hard wrapping (left: active, right: disabled)

@donokuda

This comment has been minimized.

Member

donokuda commented Dec 1, 2017

I messed around with the icon to see if it's possible to make it a little more legible and centered. I don't have push access to this repo at the moment, but I've added a diff after the screenshot to show what's changed:

screen shot 2017-12-01 at 1 41 08 pm

screen shot 2017-12-01 at 1 41 02 pm

diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js
index 27683e10..f7e5e43d 100644
--- a/lib/views/commit-view.js
+++ b/lib/views/commit-view.js
@@ -125,15 +125,15 @@ export default class CommitView {
       <div onclick={this.toggleHardWrap} className="github-CommitView-hardwrap hard-wrap-icons">
         <div className={cx('icon', 'no-hardwrap', 'icon-hardwrap-disabled', {hidden: notApplicable || hardWrap})}>
           <svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
-            <g fillRule="evenodd">
-              <path d="M6.88 9.5H6v2l-3.5-3 3.5-3v2h.88l1 1-1 1zM9 5.38V4.5h2v.88l-1 1-1-1z" />
-              <path d="M8.225 10.968L9.993 9.2l1.767 1.768.778-.778-1.768-1.768 1.698-1.697-.708-.707-1.767 1.768-1.768-1.768-.707.707 1.768 1.768-1.768 1.767z" />
+            <g fill-rule="evenodd">
+                <path d="M7.058 10.2h-.975v2.4L2 9l4.083-3.6v2.4h.97l1.202 1.203L7.058 10.2zm2.525-4.865V4.2h2.334v1.14l-1.164 1.165-1.17-1.17z"/>
+                <path fill-rule="nonzero" d="M7.842 6.94l2.063 2.063-2.122 2.12.908.91 2.123-2.123 1.98 1.98.85-.848L11.58 8.98l2.12-2.123-.824-.825-2.122 2.12-2.062-2.06z"/>
             </g>
           </svg>
         </div>
         <div className={cx('icon', 'hardwrap', 'icon-hardwrap-enabled', {hidden: notApplicable || !hardWrap})}>
           <svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
-            <path d="M11 8c0 .825-.675 1.5-1.5 1.5H6v2l-3.5-3 3.5-3v2h3v-3h2V8z" fillRule="evenodd" />
+            <path d="M11.917 8.4c0 .99-.788 1.8-1.75 1.8H6.083v2.4L2 9l4.083-3.6v2.4h3.5V4.2h2.334v4.2z" fill-rule="evenodd"/>
           </svg>
         </div>
       </div>
diff --git a/styles/commit-view.less b/styles/commit-view.less
index 824277ef..63bc9727 100644
--- a/styles/commit-view.less
+++ b/styles/commit-view.less
@@ -17,7 +17,7 @@
       position: absolute;
       right: 20px;
       bottom: 0;
-      padding: @component-padding / 2;
+      padding: (@component-padding / 5) (@component-padding / 2);
       line-height: 1;
       border: none;
       background-color: @syntax-background-color;

@kuychaco kuychaco requested a review from simurai Dec 5, 2017

@BinaryMuse

Some comments inline

@@ -380,7 +380,7 @@ export default class GitShellOutStrategy {
}
commit(message, {allowEmpty, amend} = {}) {
let args = ['commit'];
let args = ['commit', '--cleanup=strip'];

This comment has been minimized.

@BinaryMuse

BinaryMuse Dec 5, 2017

Member

Do we definitely want this all the time? For command-line Git, -m messages have cleanup set to whitespace. I guess the option would be to pass --cleanup=strip only when using the larger commit editor.... but it also seems reasonable that we'd wanna strip comments from the mini-editor too. (Do we syntax highlight the mini editor?)

This comment has been minimized.

@kuychaco

kuychaco Dec 6, 2017

Member

Yeah I think so. As of now, in the commit box we're already stripping out lines starting with #. In this way we deviate from how committing with -m works. The nice thing about doing this all the time is that we will have consistent behavior between the commit box and the expanded editor. We don't syntax highlight the mini editor.

registerTooltips() {
const expandButton = this.element.getElementsByClassName('github-CommitView-expandButton')[0];
const hardWrapButton = this.element.getElementsByClassName('github-CommitView-hardwrap')[0];

This comment has been minimized.

@BinaryMuse

BinaryMuse Dec 5, 2017

Member

Could we get these by refs instead of reaching into the element?

@simurai

simurai approved these changes Dec 6, 2017

Icon looks great..

Maybe just one question: Should toggling hard wrapping be reflected in the input? I think currently the hard wrapping is only shown in the commit, but not while you type. Is that intended?

@kuychaco

This comment has been minimized.

Member

kuychaco commented Dec 6, 2017

@simurai glad you approve!

Should toggling hard wrapping be reflected in the input? I think currently the hard wrapping is only shown in the commit, but not while you type. Is that intended?

Yeah it's somewhat tricky. Because the commit box has soft-wrapping and the width is typically smaller than the hard-wrap length of 72 characters, there's no good way of showing what the hard-wrapping will look like within the commit box. So yes, it's intended, but only because there's no great way of showing the hard-wrapping. At least not one that I can think of, but maybe you have some 馃挕s?

@simurai

This comment has been minimized.

Member

simurai commented Dec 6, 2017

Because the commit box has soft-wrapping and the width is typically smaller than the hard-wrap length of 72 characters, there's no good way of showing what the hard-wrapping will look like within the commit box.

ya, I guess it would look something like this:

asdf asdf asdf asdf asdf asdfad sfasdf asdfasd 
fasdfasdf asdf asdfasdsad
asdf asdf asdf asdf asdf asdfad sfasdf asdfasd 
fasdfasdf asdf asdfasdsad
asdf asdf asdf asdf asdf asdfad sfasdf asdfasd 
fasdfasdf asdf asdfasdsad

which is probably annoying in a narrow input. Ok, let's not do that. 馃憤

@kuychaco kuychaco merged commit 7046bc0 into master Dec 9, 2017

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

@kuychaco kuychaco deleted the ku-commit-wrap-setting branch Dec 9, 2017

@daku daku referenced this pull request Apr 16, 2018

Open

Hard line wrap #13

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