lucasruy / baunilha-modal Goto Github PK

Seria daora, o nome dessa classe ser baunilha-modal

.basic-modal-open {
    position: fixed;
    top: 50%;

Tem uma linha vazia aqui, tira ela

.basic-modal-open {
    position: fixed;
    top: 50%;
    left: 50%;
    transform: translate3d(-50%, -50%, 0px);
    display: block;
    width: 50%;
    height: 50%;
    z-index: 9999;
    background-color: white;
    padding: 20px;

    visibility: hidden;
    opacity: 0;
    user-select: none;
    transition: all 200ms ease;

Dentro do js, vc pode separar ele em uns 3 arquivos: Um pra classe BaunilhaModal, um pra Utils e um pra Interations. Pra juntar eles, você pode criar um gulpin simples.
Para interagir entre entre os arquivos vc pode colocar-los numa prop privada no Prototype do seu objeto global. Fiz um desse aqui : https://github.com/vagfsantos/github-battle-app/tree/master/src/js
Fica o desafio. Separar é bom, pra manutenção e escalabilidade. Como você criou um pluggin que pessoas podem contribuir, se todas trabalharem no mesmo arquivo, vai ser fácil ter conflitos. Quanto mais modular melhor nesse caso.

O código do método
mountModal: function(self) {
ficou muito grande. Talvez quebrar em mais funções.

Os métodos do interations, estão sendo chamados de duas formas distintas. Algumas vezes você usa o call e outras passa o this como pârametro. Seria bom fazer de uma única forma por todo o código.

Acho que o defaults poderia ficar fora do construtor como uma constante, ou num helper qualquer, algo tipo:

var BAUNILHA_DEFAULTS = {
  modalClass: 'basic-modal',
}

function getDefaults(customOptions) {
  if( customOptions ) {
    return extend(BAUNILHA_DEFAULTS, customOptions);
  }

  return extend(BAUNILHA_DEFAULTS, {});
}

A option buttonClose, poderia ser mais clara no que faz, algo como includesCloseButton ou hasCloseButton

A option appendOn ficaria mais semântica se fosse appendTo

Não fica muito claro o que significa 27 nesse trecho:
if (event.keyCode === 27) {
talvez mudar para algo como:

var KEYBOARD = {
  ESC: 27
}
if (event.keyCode === KEYBOARD.ESC) 

Tem bastante referência à classList.add no código, talvez um helper para isso, chamando assim:
addClassTo(element, className);

Essa string repete bastante também, talve criar uma variavel
('is-active');
algo como:
var classNames = {isActive: 'is-active'}

Boaaa man, ta ficando top, listei algumas melhorias. Algumas são só uns detalhes de frescura, implementa se quiser, beleza?

Pra cima, parabéns