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

Improve user experience #49

Merged

Conversation

thiagogsr
Copy link
Contributor

Melhora a experiência do usuário com as melhoras:

  • Substituição do plugin de máscara por um que ainda é mantido e funciona muito bem tanto no desktop como no mobile
  • Alteração da máscara de telefone para funcionar com 10 e 11 dígitos nos formatos 0000-0000 e 00000-0000, ao invés de 0000-00000 como está atualmente
  • Alteração dos tipos dos campos para exibir o teclado correto no mobile

Copy link
Owner

@claudiosanches claudiosanches left a comment

Choose a reason for hiding this comment

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

Nossa velho, obrigadão pela ajuda, fazia um tempo que estava procurando algo para substituir esse script e ter uma experiencia melhor em mobile.
Fiz alguns comentários de coisas que precisam ser arrumadas para funcionar bem pra todo mundo.
Se não tiver tempo de alterar essas coisas vai estar tudo bem, posso arrumar depois de fazer merge e de boa.
Obrigado novamente.

@@ -72,11 +72,11 @@ public function load_order_data( $data ) {
public function enqueue_scripts() {
$suffix = defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ? '' : '.min';

wp_register_script( 'jquery-maskedinput', plugins_url( 'assets/js/jquery-maskedinput/jquery.maskedinput' . $suffix . '.js', plugin_dir_path( __FILE__ ) ), array( 'jquery' ), '1.4.1', true );
Copy link
Owner

Choose a reason for hiding this comment

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

Talvez seria melhor manter registrando jquery-maskedinput mesmo chamando o script novo, já que não a implementação é a mesma, já que alguns outros plugins e projetos usam o script que vem deste plugin e trocar isso provavelmente iria gerar problema para umas pessoas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

O ruim é que vai carregar os dois scripts no DOM, deixando a página mais pesada. A interface dos dois plugins é a mesma: .mask. A diferença é que o maskedinput usa 9 para dígitos obrigatórios e o jquery.mask usa 9 para dígitos opcionais (no caso de máscaras numéricas), e isso vai dar conflito carregando os dois scripts.

O que sugere nesse caso?

Copy link
Owner

Choose a reason for hiding this comment

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

No caso poderia carregar duas vezes o nomes script, porque poderia carregar o jQuery.mask, mesmo chamando ele de jquery-maskedinput. E a galera teria que atualizar para carregar apenas um.
Ou chamar logo apenas de jquery-maskedinput para o novo script. Também é uma opção, não bonita, mas que não vai gerar problemas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minha preocupação em relação a isso é que pode quebrar a máscara de quem configurou seguindo a documentação do maskedinput, como o caso que falei do dígito obrigatório/opcional.

Ex: https://github.com/claudiosanches/woocommerce-pagseguro/blob/master/assets/js/transparent-checkout.js#L89

Aqui vai parar de funcionar, pois a máscara deveria ser 000.000.000-00.

Copy link
Owner

Choose a reason for hiding this comment

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

Então deixa quieto isso, melhor atualizar e notificar a galera sobre a isso xD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depois que mergear esse eu abro um no pagseguro corrigindo a máscara lá :)

);

$new_fields['billing_rg'] = array(
'label' => __( 'RG', 'woocommerce-extra-checkout-fields-for-brazil' ),
'placeholder' => _x( 'RG', 'placeholder', 'woocommerce-extra-checkout-fields-for-brazil' ),
'class' => array( 'form-row-last', 'person-type-field' ),
'required' => false
'required' => false,
'type' => 'tel'
Copy link
Owner

Choose a reason for hiding this comment

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

RG é coisa de doido, tem uns que tem letras também, além de não existe formatação que funcione para todos os estados (por isso não tem máscara também).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opa, nunca tinha visto com letra, haha, vou reverter essa parte 👍

@@ -236,6 +242,7 @@ public function checkout_billing_fields( $fields ) {
if ( isset( $fields['billing_postcode'] ) ) {
$new_fields['billing_postcode'] = $fields['billing_postcode'];
$new_fields['billing_postcode']['class'] = array( 'form-row-last', 'address-field' );
$new_fields['billing_postcode']['type'] = 'tel';
Copy link
Owner

Choose a reason for hiding this comment

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

Se usar isso com o CEP vai deixar de funcionar corretamente para quem vende dentro e fora do Brasil, o plugin suporta uma opção para escolher os campos de CPF/CNPJ se não for venda no Brasil, e ai tem vários lugares que usam letras no postcode, exemplos: Canadá e Inglaterra.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mas esse campo hoje tem máscara, permitindo apenas números, certo? Ou perdi algo nesse setup do plugin? PS: sou super noob em wordpress

Copy link
Owner

Choose a reason for hiding this comment

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

Tem máscara, mas se eu me lembro bem, alguém me fez carregar a máscara apenas se o pais é o Brasil ou eu tenho que fazer isso ainda xD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tem razão:

$( document.body ).on( 'change', '#billing_country', function() {
  if ( 'BR' === $( this ).val() ) {
    wc_ecfb_frontend.maskBilling();
  } else {
    wc_ecfb_frontend.unmaskBilling();
  }
});

Vou fazer um commit para melhorar isso 👍

Copy link
Owner

Choose a reason for hiding this comment

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

Dava para aproveitar o script ai e trocar o tipo de campo do CEP, de text para tel se for no Brasil, acho que não seria um problema em mobile.

Copy link
Contributor Author

@thiagogsr thiagogsr Mar 10, 2017

Choose a reason for hiding this comment

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

Fiz isso, aproveitei e fiz o mesmo em todos que estavam fazendo masking/unMasking, deixando com tipo tel fixo apenas cpf/cnpj: ea7473e

@thiagogsr
Copy link
Contributor Author

@claudiosanches tem previsão de quando isso virará release? Ta complicado usar no mobile :(

@claudiosanches
Copy link
Owner

@thiagogsr até este final de semana, estava tentando mexer com isso, mas com estava correndo com esse lance de mudar o WooCommerce 2.7 para 3.0.

@claudiosanches claudiosanches merged commit 4cd9aa2 into claudiosanches:master Apr 3, 2017
@claudiosanches
Copy link
Owner

@thiagogsr valeu cara, muito obrigado.

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