Skip to content

[4.0] Correcting module xtd wrong js#27499

Merged
HLeithner merged 15 commits intojoomla:4.0-devfrom
infograf768:4.0_module-xtd_js
Jan 16, 2020
Merged

[4.0] Correcting module xtd wrong js#27499
HLeithner merged 15 commits intojoomla:4.0-devfrom
infograf768:4.0_module-xtd_js

Conversation

@infograf768
Copy link
Copy Markdown
Member

@infograf768 infograf768 commented Jan 12, 2020

Summary of Changes

The admin-modules-modal.es6.js used for the module xtd was wrong as it was containing the code present in cpanel admin-add_module.es6.js

I only pasted the code from J3 here in a new admin-modules-modal.js as I fail to make it an es6.

Need help for that.
@Fedik @dgrammatiko

In the mean while, can be tested.

[EDIT] The js is now es6

Testing Instructions

Patch, run npm and then edit an article. Insert a module through the CMS Content dropdown in TinyMCE

Screen Shot 2020-01-12 at 16 17 34

Before patch

Clicking on a module in the modal does not work

After patch

Works fine

@infograf768 infograf768 requested a review from wilsonge as a code owner January 12, 2020 15:25
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Jan 12, 2020
@dgrammatiko
Copy link
Copy Markdown
Contributor

There you go:

/**
  * @copyright  Copyright (C) 2005 - 2019 Open Source Matters, Inc. All rights reserved.
  * @license    GNU General Public License version 2 or later; see LICENSE.txt
  */

document.addEventListener('DOMContentLoaded', () => {
    "use strict";

    /** Get the elements **/
    const modulesLinks = [].slice.call(document.querySelectorAll('.js-module-insert'));
    const positionsLinks = [].slice.call(document.querySelectorAll('.js-position-insert'));

    /** Assign listener for click event (for single module id insertion) **/
    modulesLinks.forEach((element) => {
        element.addEventListener('click', (event) => {
            event.preventDefault();
            const modid = event.target.getAttribute('data-module');
            const editor = event.target.getAttribute('data-editor');

            /** Use the API, if editor supports it **/
            if (window.parent.Joomla && window.parent.Joomla.editors && window.parent.Joomla.editors.instances && window.parent.Joomla.editors.instances.hasOwnProperty(editor)) {
                window.parent.Joomla.editors.instances[editor].replaceSelection("{loadmoduleid " + modid + "}")
            } else {
                window.parent.jInsertEditorText("{loadmoduleid " + modid + "}", editor);
            }
        });
    });

    /** Assign listener for click event (for position insertion) **/
    positionsLinks.forEach((element) => {
        element.addEventListener('click', function (event) {
            event.preventDefault();
            const position = event.target.getAttribute('data-position');
            const editor = event.target.getAttribute('data-editor');

            /** Use the API, if editor supports it **/
            if (window.Joomla && window.Joomla.editors && Joomla.editors.instances && Joomla.editors.instances.hasOwnProperty(editor)) {
                Joomla.editors.instances[editor].replaceSelection("{loadposition " + position + "}")
            } else {
                window.parent.jInsertEditorText("{loadposition " + position + "}", editor);
            }
        });
    });
});

@dgrammatiko
Copy link
Copy Markdown
Contributor

Actually the else part is useless in J4 so:

/**
  * @copyright  Copyright (C) 2005 - 2019 Open Source Matters, Inc. All rights reserved.
  * @license    GNU General Public License version 2 or later; see LICENSE.txt
  */

document.addEventListener('DOMContentLoaded', () => {
    "use strict";

    /** Get the elements **/
    const modulesLinks = [].slice.call(document.querySelectorAll('.js-module-insert'));
    const positionsLinks = [].slice.call(document.querySelectorAll('.js-position-insert'));

    /** Assign listener for click event (for single module id insertion) **/
    modulesLinks.forEach((element) => {
        element.addEventListener('click', (event) => {
            event.preventDefault();
            const modid = event.target.getAttribute('data-module');
            const editor = event.target.getAttribute('data-editor');

            /** Use the API **/
            if (window.parent.Joomla && window.parent.Joomla.editors && window.parent.Joomla.editors.instances && window.parent.Joomla.editors.instances.hasOwnProperty(editor)) {
                window.parent.Joomla.editors.instances[editor].replaceSelection("{loadmoduleid " + modid + "}")
            }
        });
    });

    /** Assign listener for click event (for position insertion) **/
    positionsLinks.forEach((element) => {
        element.addEventListener('click', function (event) {
            event.preventDefault();
            const position = event.target.getAttribute('data-position');
            const editor = event.target.getAttribute('data-editor');

            /** Use the API **/
            if (window.Joomla && window.Joomla.editors && Joomla.editors.instances && Joomla.editors.instances.hasOwnProperty(editor)) {
                Joomla.editors.instances[editor].replaceSelection("{loadposition " + position + "}")
            }
        });
    });
});

@dgrammatiko
Copy link
Copy Markdown
Contributor

@infograf768 my first commit build/media_src/com_modules/js/admin-modules-modal.es6.js has also some code for closing the modal, I guess you should revert to that and ignore my above comments. BTW I have no clue why they rewrote it with the new design of the template, probably you should check the dashboard after changing this file (?)

@infograf768
Copy link
Copy Markdown
Member Author

@dgrammatiko
Close button works fine with your second proposal.

BTW I have no clue why they rewrote it with the new design of the template, probably you should check the dashboard after changing this file (?)

I had checked already. It's fine.

remains to solve Hound...

@dgrammatiko
Copy link
Copy Markdown
Contributor

tabs to spaces and line length, should be easy

@infograf768
Copy link
Copy Markdown
Member Author

@dgrammatiko
yes for spaces and tabs but what about
Do not access Object.prototype method 'hasOwnProperty' from target object no-prototype-builtins ?

@dgrammatiko
Copy link
Copy Markdown
Contributor

dgrammatiko commented Jan 12, 2020

https://eslint.org/docs/rules/no-prototype-builtins
or

Object.prototype.hasOwnProperty.call(window.parent.Joomla.editors.instances, editor)

PS sorry my PC is really broken at the moment

@infograf768
Copy link
Copy Markdown
Member Author

@dgrammatiko
I think I solved all except
`Do not access Object.prototype method 'hasOwnProperty' from target object no-prototype-builtins

in line 23
&& window.parent.Joomla.editors.instances.hasOwnProperty(editor)) {

I did not understand your comment.

@hound hound bot deleted a comment from dgrammatiko Jan 12, 2020
@infograf768
Copy link
Copy Markdown
Member Author

@dgrammatiko
remains
Unexpected string concatenation prefer-template
I had tried
`Joomla.editors.instances[editor].replaceSelection('{loadposition ${position}}');
but it also throwed an error.

@infograf768
Copy link
Copy Markdown
Member Author

Maybe just
// eslint-disable-line prefer-template

what you think?

@dgrammatiko
Copy link
Copy Markdown
Contributor

It needs to be backtick not single quote!

@infograf768
Copy link
Copy Markdown
Member Author

Ok, will do tomorrow

@hound hound bot deleted a comment from dgrammatiko Jan 13, 2020
@infograf768 infograf768 changed the title [4.0] [WIP} Correcting module xtd wrong js [4.0] Correcting module xtd wrong js Jan 13, 2020
@infograf768
Copy link
Copy Markdown
Member Author

All should be OK now.
Please test.

@jwaisner
Copy link
Copy Markdown
Member

Modal now selects module after testing. Comparing to J3 the modal should close after selecting the module but does not in this PR.

@infograf768
Copy link
Copy Markdown
Member Author

Auto-close done.
Inserting module position done

@jwaisner
Copy link
Copy Markdown
Member

I have tested this item ✅ successfully on e7ef6f5


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

@N6REJ
Copy link
Copy Markdown
Contributor

N6REJ commented Jan 15, 2020

I have tested this item ✅ successfully on e7ef6f5

installed this mornings build of J4, installed patch via patch tester, ran npm ci, refreshed article, attempted to insert module & nothing happened. Tried article and it was fine.

reverted patch, reapplied patch, did NOT run npm again, and it worked fine.


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

@infograf768
Copy link
Copy Markdown
Member Author

rtc


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 15, 2020
@HLeithner HLeithner merged commit 999602b into joomla:4.0-dev Jan 16, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 16, 2020
@HLeithner
Copy link
Copy Markdown
Member

Thanks

@HLeithner HLeithner added this to the Joomla 4.0 milestone Jan 16, 2020
@infograf768 infograf768 deleted the 4.0_module-xtd_js branch January 16, 2020 09:16
brianteeman pushed a commit to brianteeman/joomla-cms that referenced this pull request Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants