WIP: code review

[guzmanpaniagua]

hola, estuve ayer en el meetup, viendo el componente me surgen las siguientes dudas:

1) ¿es una buena idea que el componente se llame paper-....? ese es el sufijo que goolge usa para su familia de componentes, se puede confundir un usuario y penar que es de google

2) existe otro componente en webcomponents.org llamado dynamicforms, que hace algo parecido, cual es la ventaja e utilizar este

3) porque hacéis esto

  isTextForm(item) {
            if (item.type === 'text-input') {
                return true;
            } else {
                return false;
            }
        }

en vez de

  isTextForm(item) {
         return item.type === 'text-input';          
        }

4) la iferencia entre los metodos optionSelectedHandler y textHandler es solo el nombre del evento, haria mas facil extender el componente si se evita la duplicacion de codigo, algo asi

      optionSelectedHandler (event){
             this.eventHandler(event.model, event.detail.value, 'option-input-response')
      },
      textHandler (event){
            this.eventHandler(event.model, event.detail.value, 'text-input-response')
        },
      eventHandler(model, value, eventName) {
            this.dispatchEvent(new CustomEvent(eventName, {
                bubbles: true,
                composed: true,
                detail:  {
                model: model,
                option: value
            }
            }));
        }

5) cuando se define la propiedad config en el codigo, solo veo que es un array, pero no hay un comentario que me explique que tipos de objetos van entro y que estructura necesito que tengan, tengo que buscarla en los comentarios del html, pero aqui tambien se usa

        static get properties() {
            return {
/*
array de configuracion que recibe la lista de campos dinamicos que se pinta,, los campos tienen esta forma 
{
    label: 'Some label',
    type: 'type-input',
    options: ['option a', 'option b', 'option c']
}
la propiedad options es opcional,
la label es el titulo del campo 
el type nos deja elegir entre inputs, select..., las claves validas son 'type-input' ......
*/
                config: {
                    type: Array
                }
            };
        }

6) es una mala practica usar tags como selectores de css paper-dropdown-menu {...}, cuando el componente evoluciona y pones un content afectas a los estilos de los componente que se puede poner dentro.

 paper-dropdown-menu {
                --paper-dropdown-menu: {
                    color: #000;
                };
            }

imagina que yo tengo otro --paper-dropdown-menu en otro lado e mi pagina, ¿esto le afecta? imagina que yo quiero que el color no sea negro¿como lo puedo cambiar?

lo ideal es ponerle clases a las cosas y en la emo dejar elegir un theme que "configure" los elementos como norma general si un padre puede afectar a los hijos, cuando queremos ver quien le pone el color xxx a un elemento tenemos que ir buscando entre todos los padres a ver quien es el que lo esta modificando, en vez de poder buscar solo en el componente o en e theme

7) usar div como selector es una mala practica, el css debe de ser indepeniente del marcado elegido

            div {
                --paper-dynamic-form-mixin: {
                    display: flex;
                    flex-direction: column;
                    justify-content: center;
                };
            }

me falta un apply, como norma general debe de haber uno para cada regla que pongas

8) mejor .nombre-mi-componente que container, aunque polymer te proteja de los demas debes de seguir usando las buenas practicas igual

  .container {
                color: var(--paper-dynamic-forms-color, #000000);
                background-color: var(--paper-dynamic-forms-bg-color, transparent);
                @apply --paper-dynamic-form-mixin;
            }

9) define variables de mixin para los colores y deja que te las cambien, primary-color, secundary...

       --paper-input-container-color: black;
                /* Label and underline color when the input is focused */
                --paper-input-container-focus-color: black;
                /* Label and underline color when the input is invalid */
                --paper-input-container-invalid-color: red;

10) items es un mal nombre, no me dice lo que es, ejor formFieldList o algo asi y usar el atributo as para decir que significa cada item indiviual, ¿un empleado, un barco...?

  <template is="dom-repeat" items="[[config]]">

11) ponerle siempre restamp a los dom-if para que no oculte el dom, sino que lo elimine

<template is="dom-if" if="[[isOptionForm(item)]]">

restamp: boolean= false When true, elements will be removed from DOM and discarded when if becomes false and re-created and added back to the DOM when if becomes true. By default, stamped elements will be hidden but left in the DOM when if becomes false, which is generally results in better performance.

bastante largo por hoy 8)

bonus game: ¿que pasa si window.customElements esta undefined?

(function PaperDynamicFormsDefinition(customElements) {
.....
    customElements.define(PaperDynamicForms.is, PaperDynamicForms);
})(window.customElements);

[alfonsorios96]

Hola, qué tal. Muchas gracias por tu asistencia y los comentarios. Son muy bien recibiros. Se están trabajando en ellos. Saludos

[alfonsorios96]

Dando respuesta puntual a cada observación:

1. No le veo problema al nombrarlos de esa manera ya que no hay ninguna regla que impida nombrarlos con el prefijo paper-, además de que se escogió así ya que está pensado en usarse como una herramienta de papel, como finalmente es un formulario.
2. Estuve revisando detenidamente el otro componente (que no conocía por cierto jeje) y la ventaja que ofrece el nuestro es que ya está construido en polymer 2 y el otro se maneja a través de web components vanilla, y no cuenta con una suite robusta de pruebas que garantiza su correcto funcionamiento.
3. No se me había ocurrido; sin embargo, se está haciendo el refactor. Gracias 👍
4. Excelente observación, se está trabajando en el refactor.
5. Se extiende la documentación en el código con base a tus comentarios. 6 -> 11. Se están trabajando los cambios en el refactor. Los cambios se verán reflejados para el tag 0.3.0 Bonus game: Basta con asegurarnos de que <link rel="import" href="../webcomponentsjs/webcomponents-lite.js"> se ejecute correctamente para que realice los polyfills necesarios. [Se agrega en el siguiente release]