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 `limit-commit-title-length` feature #2115

Merged
merged 17 commits into from Jun 23, 2019

Conversation

Projects
None yet
2 participants
@notlmn
Copy link
Contributor

commented Jun 3, 2019

Closes #1475.

  • Changed font for all commit message fields to monospace.
  • Hard-coded input fields' maxlength property to 70.
  • Adds a warning of field somehow gets filled with message longer than 70 chars.
  • Possibly also handle issue/PR title changes too?

Test

While creating a PR or merging it, the input fields do not accept characters greater than 70 characters in length.

notlmn added some commits Jun 3, 2019

function init(): void {
const inputs = select.all<HTMLInputElement>([
'#commit-summary-input', // Commit title on edit file page
'#pull_request_title', // PR title while creating PR

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jun 3, 2019

Collaborator

This is unrelated. If the user wants the PR title to match the commit they’ll do so when merging the PR 🙂

This comment has been minimized.

Copy link
@notlmn

notlmn Jun 3, 2019

Author Contributor

When creating a PR from a branch with a very long commit title, GitHub already separates out pull request title to 70 characters in title field and the rest into the message field.

So this should be a good practice for both the contributor (while opening PR) and for the owner (while merging PR) right!?

Show resolved Hide resolved source/features/limit-commit-title-length.tsx Outdated
Show resolved Hide resolved source/features/limit-commit-title-length.tsx
Show resolved Hide resolved source/features/limit-commit-title-length.tsx Outdated

notlmn added some commits Jun 3, 2019

Show resolved Hide resolved test/page-detect.ts Outdated

notlmn added some commits Jun 4, 2019

@bfred-it bfred-it self-requested a review Jun 7, 2019

@bfred-it
Copy link
Collaborator

left a comment

The actual limit seems to be 72 characters, after which GitHub clips back to 70 characters.

One thing I noticed is that validation does not apply to the initial value (or values set programmatically, like by sync-pr-title-message):

  1. Set commit title via field.value = '0'.repeat(100)
  2. Press "Commit" button
  3. The submission won't be stopped

insert-text-textarea does't trigger validation either, nor does firing input events.

Probably sync-pr-issue-title needs to be clipped to 72 characters as well, unless you find a way to trigger validation after setting the title (I couldn't. checkValidity and reportValidity report true even if the form isn't valid, which is ridiculous)

@bfred-it bfred-it added the enhancement label Jun 7, 2019

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

validation does not apply to the initial value (or values set programmatically...

The one thing I still can't figure out. But from what I've tested it looks like for the browser to validate autofilled inputs, the constraints have to be already present when the DOM is being created.

To bypass this I've tried setCustomValidity(message), which does trigger validation on form, but only until the message is reset back to normal for some reason. I don't even know if the HTML Forms spec defines this kind of behavior.

In the example screenshot below, the input wasn't touched while running the following snippets.

image

But by using setCustomValidity(), we loose a lot of default error messages provided by the user agents. And again, with resetting validation message "for some reason", we loose validation.

To be frank, @bfred-it's avatar sums up this whole situation for triggering validation for autofilled input fields.

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

The actual limit seems to be 72 characters, after which GitHub clips back to 70 characters.

I'm thinking of sticking to GitHub's convention of 70 character limit, to be consistent with GH. I can't say if this actually happens, but we might loose data when moving between 70 and 72.

@bfred-it WDYT!?

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2019

I'm thinking of sticking to GitHub's convention of 70 character limit

GitHub's limit is 72, I just tested it

But by using setCustomValidity(), we loose a lot of default error messages provided by the user agents. And again, with resetting validation message "for some reason", we loose validation.

Definitely, I like that the browsers say "it should be 72, but now there are 76", which we wouldn't get with pattern for example.

setCustomValidity lets the developer tell the browser whether it's valid or not, so if you pass a truthy string it will interpret it as "the developer said it's invalid, and the string is the error to show the user." It's the opposite of what we want

To be frank, @bfred-it's avatar sums up this whole situation for triggering validation for autofilled input fields.

Hehe yeah, setting the value programmatically breaks the validation altogether, so I think the only option we have is to trim sync-pr-title-commit

Limit merge title length in `sync-pr-commit-title`
- Extend commit title length from 70 to 72
@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

setCustomValidity lets the developer tell the browser whether it's valid or not, so if you pass a truthy string it will interpret it as "the developer said it's invalid, and the string is the error to show the user." It's the opposite of what we want

I should've read the spec for that one, turned out to be completely different from what I thought it would be.


@bfred-it take a look at 739fcd5, as we are already limiting PR title length to 72 characters when the PR is being created there should not be many issues here. But we might also need to extend this to limit to editing PR and issue titles too.

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jun 19, 2019

Ok, we should restore the JS validation, but it can be very simple:

On input
  setValidity(field.length > limit && `The title should not be longer than 72 characters, but is currently ${field.length}`)

Almost one line

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Output for the last commit, while editing a file (tested with disabling maxlength):

image

Is this okay?

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jun 19, 2019

Uhm, the suggestion I gave you was literally one line of code inside the listener. No need to make it more complex I think.

Perhaps the same function needs to be run when the form is submitted as well, as long as it blocks the submission when it fails.

Also we can drop the maxlength attribute altogether since we now have JS validation. It makes it easier to edit the text in it

Likewise we can drop the PR title limit altogether. This feature will error on merge, if the merger has RGH and the feature is enabled, rather than by the PR sender

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2019

The title should be maximum 72 characters, but is 139

notlmn added some commits Jun 20, 2019

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Perhaps the same function needs to be run when the form is submitted as well, as long as it blocks the submission when it fails.

Also we can drop the maxlength attribute altogether since we now have JS validation. It makes it easier to edit the text in it

Likewise we can drop the PR title limit altogether. This feature will error on merge, if the merger has RGH and the feature is enabled, rather than by the PR sender

All done!


The title should be maximum 72 characters, but is 139

image

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

Doesn't work correctly in Firefox: the submission is stopped but the button still changes to "Merging". Also no error message is shown. setCustomValidity sets the validity but then we need to call reportValidity to show the message.

We probably need to catch the click on "Merge" instead.

Validate inputs on opening merge box
This also validates input values automatically restored by GitHub on page load for merge box
for (const inputField of inputs) {
inputField.dispatchEvent(new Event('input'));
}
});

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jun 23, 2019

Collaborator

Good idea, I wonder if this makes the submit handler unnecessary. I don't think the title field is ever programmatically changed when the PR merge panel is open; even GitHub specifically avoids this.

In that case, validityCallback can be simplified as I had first suggested, likely.

Also this event only needs to be triggered on the title field because that's what this feature cares about.

This comment has been minimized.

Copy link
@notlmn

notlmn Jun 23, 2019

Author Contributor

In that case, validityCallback can be simplified as I had first suggested, likely.

Yup!


Also this event only needs to be triggered on the title field because that's what this feature cares about.

The merge input field can be on index 0 or 1 of the inputFields variable when querying DOM. Instead of searching for it, I just figured it'd be easy to trigger input events on all of them. Doesn't matter that much anyway.

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jun 23, 2019

Collaborator

I had assumed that fields was [title, message] but I think it's always just one; the double selector made me think it was two. I think you just need a select and you can drop the loop

notlmn added some commits Jun 23, 2019

@bfred-it bfred-it changed the title Limit commit title input field length Add `limit-commit-title-length` feature Jun 23, 2019

@bfred-it bfred-it merged commit 3852f38 into sindresorhus:master Jun 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jun 23, 2019

:neato: 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.