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.

Sign up for GitHub

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

Jump to bottom

Tidy writeDynaList() #12184

Merged
merged 7 commits into from
Apr 20, 2018
Merged

Tidy writeDynaList() #12184

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5d9f7a9
Cleaned writeDynaList() in core.js
milux Sep 27, 2016
5fbdbba
Removed explanation comments
milux Sep 27, 2016
44f3764
removed all API changes
milux Oct 3, 2016
3d0bccd
Merge remote-tracking branch 'upstream/staging' into staging
milux Nov 7, 2016
a4759c5
Merge branch 'staging' into staging
rdeutz May 27, 2017
712332f
Merge remote-tracking branch 'upstream/staging' into staging
milux Nov 1, 2017
df4e057
updated compressed core.js
milux Nov 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 10 additions & 26 deletions media/system/js/core-uncompressed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ Joomla.editors.instances = Joomla.editors.instances || {};
};

/**
* USED IN: administrator/components/com_modules/views/module/tmpl/default.php
* USED IN: media/system/js/moduleorder.js
*
* Writes a dynamically generated list
*
Expand All @@ -348,42 +348,26 @@ Joomla.editors.instances = Joomla.editors.instances || {};
* @param string
* The key to display for the initial state of the list
* @param string
* The original key that was selected
* @param string
* The original item value that was selected
* @param string
* The elem where the list will be written
* The element where the list will be inserted
*/
window.writeDynaList = function ( selectParams, source, key, orig_key, orig_val, element ) {
var html = '<select ' + selectParams + '>',
hasSelection = key == orig_key,
i = 0,
selected, x, item;

for ( x in source ) {
if (!source.hasOwnProperty(x)) { continue; }
window.writeDynaList = function ( selectParams, source, key, orig_val, element ) {
var html = '<select ' + selectParams + '>', i, selected, item;

item = source[ x ];
for ( i = 0; i < source.length; i++ ) {
item = source[ i ];

if ( item[ 0 ] != key ) { continue; }

selected = '';
selected = orig_val == item[ 1 ] ? ' selected="selected"' : '';

if ( ( hasSelection && orig_val == item[ 1 ] ) || ( !hasSelection && i === 0 ) ) {
selected = 'selected="selected"';
}

html += '<option value="' + item[ 1 ] + '" ' + selected + '>' + item[ 2 ] + '</option>';

i++;
html += '<option value="' + item[ 1 ] + '"' + selected + '>' + item[ 2 ] + '</option>';
}

html += '</select>';

if (element) {
element.innerHTML = html;
} else {
document.writeln( html );
}
element.innerHTML = html;
Copy link
Contributor

Choose a reason for hiding this comment

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

what will happen if someone call this function without setting the element?

Copy link
Contributor Author

@milux milux Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The same as with every other function that is not called according to its definition: It will fail to do its job. In this case, the result is an unchanged DOM.
I removed the else branch because the current implementation is only used in a place where the element is specified, and document.write(ln) is one of the most awful things ever seen in JS, comparable to eval(). Such functions do only cause headache and should imho die out asap.

Copy link
Contributor

Choose a reason for hiding this comment

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

@milux I will totally agree with you but, (in Joomla there is almost always a but) the core.js API cannot change because there might be some 3rd PD that uses it and depends on the writeln(), so if that is changed a B/C break is introduced. Same goes for the params. I could have done this when I did #11040 but unfortunately we can't break B/C in minor versions.
You can rebase this PR against 4.0-dev branch, as the backwards compatibility is not an issue there

Copy link
Contributor Author

@milux milux Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I see... you're right, I didn't consider that this might be used by any 3rd party stuff. (I doubt it, to be honest, but when you guarantee API stability...).
Fine, I will redo anything that changed the public API, some changes might still be useful.

};

/**
Expand Down
2 changes: 1 addition & 1 deletion media/system/js/moduleorder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
for (i = 0; i < response.data.length; ++i) {
$orders[i] = response.data[i].split(',');
}
writeDynaList('name="' + $name + '" id="' + $id +'"' + $attr, $orders, $originalPos, $originalPos, $originalOrder, $element);
writeDynaList('name="' + $name + '" id="' + $id +'"' + $attr, $orders, $originalPos, $originalOrder, $element);
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you dropping one viable here? The function needs 6 vars and you end up using 5? This looks wrong...

Copy link
Contributor Author

@milux milux Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Edit: Fixed, see below.

}
}

Expand Down