Joomla! Issue Tracker - CMS

Download
Launch
Feeling Lucky

[#30171] - [4.0] replace form in table by form in control-groups

NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar hans2103
hans2103
23 Jul 2020

Pull Request for Issue #29901 .

Summary of Changes

This PR replaces the form-in-table by a form with decent control-groups

Testing Instructions

  • After applying patch running npm run build:js is required
  • Go to Global Configuration > Joomla! Update
  • Select Custom URL, Development, and enter https://update.joomla.org/core/nightlies/next_major_list.xml
  • Go to System > Joomla Update
  • Click Live Update tab
  • View source and inspect the HTML around the input fields

Actual result BEFORE applying this Pull Request

Schermafdruk 2020-07-23 08 56 44

Expected result AFTER applying this Pull Request

Schermafdruk 2020-07-23 08 55 33

Documentation Changes Required

avatar hans2103 hans2103 - open - 23 Jul 2020
avatar hans2103 hans2103 - change - 23 Jul 2020
Status New ⇒ Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Jul 2020
Category ⇒ Administration com_joomlaupdate
avatar hans2103 hans2103 - change - 23 Jul 2020
Title
…
replace form in table by form in control-groups
[4.0] replace form in table by form in control-groups
avatar hans2103 hans2103 - edited - 23 Jul 2020
avatar brianteeman brianteeman - test_item - 24 Jul 2020 - Tested unsuccessfully
avatar brianteeman
brianteeman - comment - 24 Jul 2020

I have tested this item ? unsuccessfully on 44f0d9e

After applying the PR the source does look like the result in the original post. but as soon as you select ftp or hybrid from the Installation method select you will see that a bunch of table classes are applied and the inline style of display:none is not removed so you cant actually use the fields

default

image

hybrid

image

And then if you go back to the default method you will see an additional class of hidden is added
image

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30171.
avatar hans2103 hans2103 - change - 24 Jul 2020
Labels Added: ?
avatar hans2103
hans2103 - comment - 24 Jul 2020

I see it happening too... now I have to find out what script is causing this. Help is welcome on this one.

avatar hans2103
hans2103 - comment - 24 Jul 2020

found something:

joomla-cms/build/media_source/com_joomlaupdate/js/default.es5.js

Lines 11 to 19 in 0a611c4

Joomla.extractionMethodHandler = function(element, prefix) {
var displayStyle = element.value === 'direct' ? 'hidden' : 'table-row';
document.getElementById(prefix + '_hostname').classList.add(displayStyle);
document.getElementById(prefix + '_port').classList.add(displayStyle);
document.getElementById(prefix + '_username').classList.add(displayStyle);
document.getElementById(prefix + '_password').classList.add(displayStyle);
document.getElementById(prefix + '_directory').classList.add(displayStyle);
}

avatar joomla-cms-bot joomla-cms-bot - change - 24 Jul 2020
Category Administration com_joomlaupdate ⇒ Administration com_joomlaupdate JavaScript Repository NPM Change
avatar hans2103 hans2103 - change - 24 Jul 2020
The description was changed
avatar hans2103 hans2103 - edited - 24 Jul 2020
avatar hans2103
hans2103 - comment - 24 Jul 2020

Addition for test instructions:

  • After applying patch running npm run build:js is required
avatar Quy Quy - test_item - 24 Jul 2020 - Tested successfully
avatar Quy
Quy - comment - 24 Jul 2020

I have tested this item ✅ successfully on 40fa3ce

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30171.

avatar Quy
Quy - comment - 24 Jul 2020

See #30185 for issue switching between the methods.

avatar brianteeman
brianteeman - comment - 24 Jul 2020

Sorry but I still dont think this is correct.

On page load we have
<div class="control-group" id="row_ftp_hostname" style="display: none">

When I change to hybrid or ftp we get the js error so cant tell what is supposed to happen to the markup there as nothing changes
But when you change back to Write files directly you get

<div class="control-group hidden" id="row_ftp_hostname" style="display: none">

It cannot be correct to have a class of hidden AND an inline style of display: none

The JS error is related to all of this. You cant fix them individually

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30171.
avatar hans2103 hans2103 - change - 24 Jul 2020
Labels Added: NPM Resource Changed
avatar hans2103
hans2103 - comment - 24 Jul 2020

the inline styling style="display:none;" is set through the following PHP echo :

<?php echo $this->ftpFieldsDisplay; ?>

That part is coming from

joomla-cms/administrator/components/com_joomlaupdate/src/View/Joomlaupdate/HtmlView.php

Line 143 in 06bf70c

$this->ftpFieldsDisplay = $this->ftp['enabled'] ? '' : 'style = "display: none"';

Since I don't have ftp activated I can see the JS error:

default.min.js?b1a4c7f1a3537b0e17f61299c4151088:1 Uncaught DOMException: Failed to execute 'add' on 'DOMTokenList': The token provided must not be empty.
    at Object.t.extractionMethodHandler (http://joomla4.test/media/com_joomlaupdate/js/default.min.js?b1a4c7f1a3537b0e17f61299c4151088:1:172)
    at HTMLSelectElement.<anonymous> (http://joomla4.test/media/com_joomlaupdate/js/default.min.js?b1a4c7f1a3537b0e17f61299c4151088:1:1410)
t.extractionMethodHandler @ default.min.js?b1a4c7f1a3537b0e17f61299c4151088:1
(anonymous) @ default.min.js?b1a4c7f1a3537b0e17f61299c4151088:1

In the JS change I've changed 'table-row' into ''.|
It seems that I cannot add an empty something.
I've updated my PR by adding an extra check if the class hidden can be set or not. It will only execute when it is not false.

avatar Quy
Quy - comment - 24 Jul 2020

So Hybrid and Write files using FTP should not be listed when FTP is disabled.

avatar brianteeman
brianteeman - comment - 24 Jul 2020

@Quy look at how it works in j3

avatar hans2103
hans2103 - comment - 25 Jul 2020

I am closing this issue in favor of #30168 (comment)

avatar hans2103 hans2103 - change - 25 Jul 2020
Status Pending ⇒ Closed
Closed_Date 0000-00-00 00:00:00 ⇒ 2020-07-25 13:41:44
Closed_By ⇒ hans2103
avatar hans2103 hans2103 - close - 25 Jul 2020

Add a Comment

Login with GitHub to post a comment