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
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Validate inputs on opening merge box

This also validates input values automatically restored by GitHub on page load for merge box
  • Loading branch information...
notlmn committed Jun 22, 2019
commit 67ea2fcc20b42767ed8c34ed103247727ed29943
@@ -1,6 +1,7 @@
import './limit-commit-title-length.css';
import select from 'select-dom';
import features from '../libs/features';
import onPrMergePanelOpen from '../libs/on-pr-merge-panel-open';

function init(): void {
const inputs = select.all<HTMLInputElement>([
@@ -27,6 +28,14 @@ function init(): void {
inputField.addEventListener('input', validityCallback);
This conversation was marked as resolved by bfred-it

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jun 3, 2019

Collaborator

I don’t think we need a custom UI for this. Can you ensure that the browser triggers the regular form validation? It’ll say “the field is longer than allowed” without any additional JS

Actually this would be even better than limiting with maxlength because maxlength makes it hard to edit the field.

This comment has been minimized.

Copy link
@notlmn

notlmn Jun 3, 2019

Author Contributor

Yes, GitHub validates all inputs before submitting using the forms validate attribute. But I think there is no way for us to trigger the browsers popup by our self.

The popup appears only while trying to submit the form, not while editing the field. For comparison, GitHub shows it's own warning message while typing, but that's not a strict enforcement anyway.

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jun 3, 2019

Collaborator

think there is no way for us to trigger the browsers popup by our self.

The popup naturally comes up when validation is required, when the form is submitted, but I don't know if GitHub is using novalidate or something

This comment has been minimized.

Copy link
@notlmn

notlmn Jun 3, 2019

Author Contributor

Yes, we can set custom error message using setCustomValidity(), but I wanted a warning while the user was typing. The custom validation message only appears when form is being submitted.

If that's too much of code to have for too little use, I'd be happy to drop it.

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jun 3, 2019

Collaborator

but I wanted a warning while the user was typing

Not a bad idea, but I wouldn't want to make the area too busy, we already have some small text there from sync-pr-commit-title

How about using some :invalid style like in options.css?

This comment has been minimized.

Copy link
@notlmn

notlmn Jun 4, 2019

Author Contributor

image

😅

inputField.form!.addEventListener('submit', validityCallback);
}

// For PR merges, GitHub restores any saved commit messages on page load
// Triggering input event for these fields immediately validates the form
onPrMergePanelOpen(() => {
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

}

features.add({
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.