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

[4.0] Correcting module xtd wrong js #27499

Merged
merged 15 commits into from Jan 16, 2020

Conversation

infograf768
Copy link
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

@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
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
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
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
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
Contributor

tabs to spaces and line length, should be easy

@infograf768
Copy link
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
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
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
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
Member Author

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

what you think?

@dgrammatiko
Copy link
Contributor

It needs to be backtick not single quote!

@infograf768
Copy link
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
Member Author

All should be OK now.
Please test.

@jwaisner
Copy link
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
Member Author

Auto-close done.
Inserting module position done

@jwaisner
Copy link
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
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
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
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.

None yet

6 participants