squash changes v1.0 #1
Conversation
There was a problem hiding this comment.
Está genial el laburo del componente, @fallemand. Hay cosas que se pueden hacer con un poco más de claridad o más simples, te dejé un par de comments. ^^
| <form> | ||
| <div class="ui-telephone"> | ||
| <div class="ui-telephone__number"> | ||
| <input id="telephone" type="text" /> |
There was a problem hiding this comment.
Ojo con los selectores #id. Siempre es preferible (para componentes UI especialmente) utilizar [data-js] como selector para integraciones con JavaScript, como sugiere este artículo.
| @@ -0,0 +1,11 @@ | |||
| { | |||
| 'required': 'Completa este dato.', | |||
There was a problem hiding this comment.
Para documentos JSON se utiliza " en vez de '. Más información acá.
| this.countriesList = require('./country-list'); | ||
| this.field = arguments[0]; | ||
| this.classNames = { | ||
| tel : 'ui-telephone', |
There was a problem hiding this comment.
En este caso, como classNames funciona como un hash map, las claves deberían estar entrecomilladas y no irían los espacios antes del :.
| this.field = arguments[0]; | ||
| this.classNames = { | ||
| tel : 'ui-telephone', | ||
| telError : ' ui-telephone--error', |
There was a problem hiding this comment.
Los espacios delante del nombre de la clase pueden evitarse haciendo a la hora de implementar algo como esto:
['tel', 'telSuccess'].map(function(k) {
return component.classNames[k];
}).join(' '); // returns 'ui-telephone ui-telephone--success'| //---------------------------------------------------- | ||
| // Define our constructor | ||
| //---------------------------------------------------- | ||
| global.jsTelephoneInput = function() { |
There was a problem hiding this comment.
Se podrían enumerar directamente los parámetros en vez de depender de arguments que le quita un poco de legibilidad al constructor.
global.jsTelephoneInput = function(field, settings) {
// ...| }; | ||
|
|
||
| //Checks parameters written as html attributes | ||
| for (var parameter in defaults) { |
There was a problem hiding this comment.
Para iterar en un objeto, es más seguro utilizar Object.keys para no leer propiedades de forma inesperada (for..in entra al prototype).
| variable = validation.innerHTML.substring(start + 2, end); | ||
| validation.innerHTML = (start > -1) ? validation.innerHTML.replace('##' + variable + '##', getTextToRepalce(variable) ) : validation.innerHTML; | ||
|
|
||
| function getTextToRepalce(variable) { |
There was a problem hiding this comment.
Acá debería ser getTextToReplace.
|
|
||
| // Creates list of countries | ||
| function createCountriesList(elements) { | ||
| for (var i = 0; i < component.countriesList.length; i += 1) { |
There was a problem hiding this comment.
Esto también se puede resolver utilizando Array.forEach, que ya tiene soporte completo en todos los browsers, pudiendo hacer:
component.countriesList.forEach(function(country) {
// ...
listItem.innerHTML += '<span class="' + component.classNames.telFlagsListItemName + '">' + country[0] + '</span>';
// ...
});| top: 50%; | ||
| right: 0; | ||
| border: 4px solid transparent; | ||
| content: ""; |
There was a problem hiding this comment.
¿Para qué figura content: "" si no se asignada nada?
There was a problem hiding this comment.
@joelalejandro El content debe estar, sino no se renderiza el elemento.
| &.ui-telephone__flag-icon--np { | ||
| background-color: transparent; | ||
| } | ||
| } |
There was a problem hiding this comment.
Aparentemente el sprite de flags incluye muchísimas banderas que no usamos. ¿Se podrían sacar?
| </div> | ||
| </form> | ||
|
|
||
| <h3>Resultado</h3> |
There was a problem hiding this comment.
Ojo que no existen ni el <h1> ni el <h2>. Para que el contenido sea accesibilidad es necesario que el orden de los headings sea natural (<h1>, <h2>, <h3>, etc...).
There was a problem hiding this comment.
Agrego, tiene que haber solamente un h1 por template, el resto lo podes manejar.
| <head> | ||
| <meta charset="utf-8"> | ||
| <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1"> | ||
| <title>Telephone </title> |
There was a problem hiding this comment.
Si el lang es "es", no debería decir "Teléfono"? O podría ser algo más descriptivo onda "Validación internacional de Teléfono con JS"
| <meta name="description" content=""> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1"> | ||
|
|
||
| <link href="https://http2.mlstatic.com/secure/org-img/ch/ui/1.1.1/chico.min.css" rel="stylesheet" type="text/css"> |
There was a problem hiding this comment.
Podrías usar la última versión de chico? 2.0.5 sino me equivoco...
|
|
||
| //Check if parameters exists. | ||
| if(!arguments[0] || !arguments[1] || typeof arguments[1] !== "object") { | ||
| console.log('js-telephone-input: field or parameters are not defined'); |
There was a problem hiding this comment.
@HGARZON ahora ví este - es un control de errores, la otra alternativa es tirar un throw new Error('...').
fallemand
force-pushed
the
version1.0
branch
from
280ceb5 to
04db588
Compare
| </thead> | ||
| <tbody> | ||
| <tr> | ||
| <td>required</td> |
There was a problem hiding this comment.
Podes agregar scope="row" como hiciste a los th pero aplicado a los td
Issue
Dejo en el PR solo los cambios realizados sobre la base generada por el ui-component.