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

Fix title not setting custom error messages; update documentation #13

Merged
merged 2 commits into from
Jun 22, 2015

Conversation

srsgores
Copy link
Contributor

I've updated the addon so that rather than just <input type="radio" /> buttons having error messages customized by their title attributes, all inputs will share this behaviour.

I have outlined all of this, and more, in the readme.md file.

I have also fixed a number of typos and grammatical errors, as well as provided some useful examples.

@@ -165,7 +165,7 @@ export default Ember.Mixin.create({
} else {
parent.addClass('has-error');
element.next('.input-error').remove();
element.after('<p class="input-error" role="alert">' + errorMessage + '</p>');
element.after(`<label class="input-error" role="alert" for="${element.attr("id")}">${errorMessage}</label>`);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure about the label tag here. Isn't this label only to be used on par with an input? That's not really the case here. For the ${} syntax, what's the browser support of it (I think this is ES6 syntax and is converted to ES5 in Ember-CLI right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bakura10, yes. And since all error messages are for input controls, the label is the safest, most accessible choice. The label is better than any other element because is automatically focuses the corresponding input, select, or textarea element when clicked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

labels should be used for all form controls: select, textarea, and input elements. The W3c spec will explain more.

@bakura10
Copy link
Member

That looks good! I'm not doign any EmberJS work at the moment, but did you try it with latest version of Ember? Does it still work even in 1.13 with Glimmer? I think that the library could be done better now with the new tools (especially about a more extensible way to render errors... i don't like the fact that people are stuck with the pre-defined markup, either being a or a

this lacks flexibility... but at the same time I don't want to have custom input elements, as I like the idea of this library being completely plug and play and not requiring changes to the form code.

@srsgores
Copy link
Contributor Author

@bakura10, I've tested with the latest non-glimmer EmberJS. As in, I have tested on ember-cli 0.2.7, and this works. I am using these same fixes on a production ember site I am currently developing.

@srsgores
Copy link
Contributor Author

Are we still merging this?

@bakura10
Copy link
Member

My mistake. Sure.

bakura10 added a commit that referenced this pull request Jun 22, 2015
Fix title not setting custom error messages; update documentation
@bakura10 bakura10 merged commit 7f76d60 into maestrooo:master Jun 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants