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

Made %s string formatting tokens optional in field validator error messages. #99

Merged
merged 1 commit into from
Mar 12, 2014

Conversation

Flaise
Copy link
Contributor

@Flaise Flaise commented Mar 9, 2014

can you provide more info about your use case? With regard to "any sensibly laid out form already makes it obvious what field an error refers to", not only are most forms not necessarily sensibly laid out, but clear text is absolutely critical for accessibility - how can a blind user, for example, know based on the layout which field an error refers to?

If an HTML form is unable to convey clearly which error goes with which field then either it is being read by a text-to-speech interface or it is in need of a redesign. In the case of the former, you already have defaults designed for this purpose. In the case of the latter, it's not yours or any library's job to undo a programmer's mistakes. Also, when I make a website for art or video games, I shouldn't and don't need to care whether blind people can use the site.

One of the more natural and flexible ways to render one of your forms is with the following Jade and CSS code:

mixin form_normal(form)
    - var allRequired = true
    - for(var k in form.fields)
        - if(!form.fields[k].required)
            - allRequired = false

    .normalform
        each field in form.fields
            .row
                label(for=field.name)
                    != field.labelText()
                    if field.required && !allRequired
                        | *
                .io
                    != field.widget.toHTML(field.name, field)
                    .error
                        = field.error
        if !allRequired
            .helptext.
                Fields marked with * are required.
.normalform {
    margin-bottom: .75em;
}
.normalform label {
    display: table-cell;
    padding-right: .5em;
    font-size: .9em;
}
.normalform input {
    display: table-cell;
    border: 2px inset #3b7d96;
    width: 20em;
    background-color: #fafafa;

    -webkit-box-sizing: border-box;
    -moz-box-sizing: border-box;
    box-sizing: border-box;
}
.normalform .row {
    display: table-row;
}
.normalform .io {
    display: table-cell;
    padding-bottom: 1em;
}
.normalform .error {
    color: #ea0806;
    font-weight: bold;
}
.normalform .helptext {
    font-size: .7em;
    color: #686868;
}

The result is this:

image

It may not be the prettiest form but it's user-friendly. This may not be the best way to render it but it's a good one. Overall, it's a pretty good use case. This is what it looks like with the error messages made possible in my branch:

image

Notice the error message with the field name nowhere in it yet still remaining perfectly descriptive in any kind of browser. The default would be "Does not match password.", which sounds robotic and/or foreign mostly because there are so many better ways to word that.

Granted, I could rename the password field so the message doesn't look so robotic but that restricts the grammar I can use and requires me to access the field's data with bracket syntax whenever I want to use spaces. Not to mention how obfuscatory it is to have such an apparently illogical identifier in your codebase. This kind of constraint also makes your library unsuitable for rendering more than one form within the same form tag - which is sometimes the most modular and maintainable way to design an interface - because of possible name collisions. If I wanted to name a field 'form_0_name', the resulting error message would be remarkably offputting to pretty much all users.

@@ -3,6 +3,14 @@

var util = require('util');

function format (message) {
if (arguments.length < 2 || message.lastIndexOf('%s') >= 0) {
return util.format.apply(null, [message].concat(Array.prototype.splice.call(arguments, 1)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

splice mutates the object in-place - can you use slice here instead?

@ljharb
Copy link
Collaborator

ljharb commented Mar 12, 2014

Thanks! I left some comments, and I'm planning on accepting this. Please rebase your branch on top of the latest master, as the underlying test framework has shifted to tape - although the syntax is almost the same.

@Flaise
Copy link
Contributor Author

Flaise commented Mar 12, 2014

Updated; take another look.

...Whoever invented those string segmentation functions and named them slice and splice is evil and needs to go sit in the corner and think about what he's done.

@@ -250,14 +250,14 @@ test('color', function (test) {
});

test('alphanumeric', function (test) {
function makeTest(message, data, expected) {
var makeTest = function (message, data, expected) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

touché :-)

ljharb added a commit that referenced this pull request Mar 12, 2014
Made `%s` string formatting tokens optional in field validator error messages.
@ljharb ljharb merged commit 43949ac into caolan:master Mar 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants