[wp-trac] [WordPress Trac] #34376: Review the FTP credentials form

[WordPress Trac]

#34376: Review the FTP credentials form
----------------------------+--------------------------------------------
 Reporter:  afercia         |       Owner:  mte90
     Type:  defect (bug)    |      Status:  assigned
 Priority:  normal          |   Milestone:  4.6
Component:  Administration  |     Version:  4.2
 Severity:  normal          |  Resolution:
 Keywords:  has-patch       |     Focuses:  ui, accessibility, javascript
----------------------------+--------------------------------------------

Comment (by afercia):

 Thanks very much @Mte90. Some feedback on the patch.

 My bad, maybe in my ticket description the words "above all the other
 fields" are a bit confusing, I meant to move the "Connection Type" radio
 buttons above the "Authentication Keys" fields but below the Username and
 Password fields. I'd recommend this order:
 - Hostname
 - Username
 - Password
 - Connection Type
 - and if SSH2 is selected: Authentication Keys
 This way, when users change the selected radio buttons, the Authentication
 Keys fields will collapse/expand '''below''' the radio buttons

 The toggled element should include the span with the text "Enter the
 location on the server where the public ..." so maybe wrap everything in a
 div?

 [[Image(https://cldup.com/foYgmVGutV.png)]]

 [[Image(https://cldup.com/1jv2EAMIuP.png)]]

 About the JS part, there are now both click and change events attached on
 the radio buttons and they call functions meant to do the same thing

 [[Image(https://cldup.com/bchVuFZpkL.png)]]

 The `click` event comes from the old inline JS now moved in `updates.js`
 {{{
 jQuery("#ssh").click(function () {
         jQuery("#ssh_keys").show();
 });
 jQuery("#ftp, #ftps").click(function () {
         jQuery("#ssh_keys").hide();
 });
 jQuery('#request-filesystem-credentials-form
 input[value=""]:first').focus();
 }}}

 The `change` event comes from
 {{{
 // Hide SSH fields when not selected
 $( '#request-filesystem-credentials-dialog input[name="connection_type"]'
 ).on( 'change', function() {
         $( this ).parents( 'form' ).find( '#private_key, #public_key'
 ).parents( 'label' ).toggle( ( 'ssh' == $( this ).val() ) );
 }).change();
 }}}

 I'd keep the `change` event and in the first block keep just the line to
 set initial focus on the first form field and remove all the rest.
 The second block should be adjusted to toggle the entire fieldset and the
 description text (it currently targets only the public and private fields
 labels).
 Also, it currently works only in the modal, the jQuery selector for `on()`
 should be adjusted in order to work also in the Updates page. I'd probably
 just remove `#request-filesystem-credentials-dialog` unless I'm missing
 something.

 Lastly, it would be a nice opportunity for some coding standards, but only
 on the changed lines please :)

--
Ticket URL: <https://core.trac.wordpress.org/ticket/34376#comment:8>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform