Skip to content

Comments

[4.0] Revert to jQuery in menus module selection till CE modals are used#23062

Merged
wilsonge merged 8 commits intojoomla:4.0-devfrom
Digital-Peak:j4/fix/menu/reload
Dec 13, 2018
Merged

[4.0] Revert to jQuery in menus module selection till CE modals are used#23062
wilsonge merged 8 commits intojoomla:4.0-devfrom
Digital-Peak:j4/fix/menu/reload

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Nov 13, 2018

Pull Request for Issue #22967.

Summary of Changes

Revert to jQuery till the modal in the menus module selection is created by a custom element. Till then it fixes the JS error mentioned in #22967 and reloads the page after the modal gets closed.

Testing Instructions

  • Open /administrator/index.php?option=com_menus&view=menus
  • Click on Modules in the "Linked modules" column
  • Click on the first entry
  • Wait till the modal has opened
  • Click on cancel

Expected result

The page reloads after a second.

Actual result

The page does not reload.

@dgrammatiko
Copy link
Contributor

@laoneo please add the mutataion observer as well otherwise this will be broken once you move to custom elements

@laoneo
Copy link
Member Author

laoneo commented Nov 13, 2018

Will it not be broken anyway?

@dgrammatiko
Copy link
Contributor

Just use the snippet I gave you

@laoneo
Copy link
Member Author

laoneo commented Nov 13, 2018

I'm not addung somwrhing which is not needed qhen we have the jQuery dep. anyway. We can do it right when we move to CE.

@dgrammatiko
Copy link
Contributor

Then when that code will be vanilla it will be broken. Jquery is something used here just because the event is not parpagating to the window. Also if you don’t commit that code I guess you will have to find someone that understands how to fix that problem later on, which, in the j-world is not very easy. Anyways you asked me and I provided you the snippet, if you’re not gonna use it I guess I just wasted my time once again

@Fedik
Copy link
Member

Fedik commented Nov 14, 2018

@laoneo the problem here that it will reload a page, doesn't matter which modal was closed. That unexpected behavior.

@dgrammatiko I think it can be more simple

In general the bug because of incorrect use of Joomla.Modal.getCurrent().

Please hold this, for now, (not close).
I will try check more, when will get some time

@dgrammatiko
Copy link
Contributor

That unexpected behavior.

@Fedik actually the part of the page with the modules should be Ajax driven and in that case the page reload is useless. But requires some heavy refactoring.

In general the bug because of incorrect use of Joomla.Modal.getCurrent().

Unfortunately the event (bs.hidden in our case) is tightly coupled to the modal element, it's not a document event (and honestly specificity is good).

@dgrammatiko
Copy link
Contributor

@Fedik actually there is no need for mutation observers here:

        const modals = [].slice.call(document.querySelectorAll('.joomla-modal'));

        modals.forEach((modal) => {
            console.log(modal)
            modal.addEventListener('hide.bs.modal', () => {
                setTimeout(() => { window.location.reload(); }, 1000);
            });
        })

@C-Lodder
Copy link
Member

@dgrammatiko even better to use a Promise

@dgrammatiko
Copy link
Contributor

@C-Lodder the problem is that Bootstrap's events do fire in the jQuery context: https://codepen.io/dgrammatiko/pen/bQqzGx

Other than that Joomla hardly ever does anything in the client side, almost all of the html is server side rendered thus mutation observers and promises are hardly required. Exception here the custom elements that actually ALL of them needs to be rewritten with the usage of mutation observers!

@dgrammatiko
Copy link
Contributor

To recap here:
Replace lines 21-24 in the admin-menu script with:

        const modals = [].slice.call(document.querySelectorAll('.joomla-modal'));

        modals.forEach((modal) => {
            console.log(modal)
            modal.addEventListener('hide.bs.modal', () => {
                setTimeout(() => { window.location.reload(); }, 1000);
            });
        })

and also patch the bootstrap-init in the modal part so that events are actual javascript events and not only jQuery:

        if (modal.length) {
            $.each($('.joomla-modal'), function() {
                var $self = $(this);

                // element.id is mandatory for modals!!!
                var element = $self.get(0);

                // Comply with the Joomla API
                // Bound element.open()
                if (element) {
                    element.open = function () {
                        return $self.modal('show');
                    };

                    // Bound element.close()
                    element.close = function () {
                        return $self.modal('hide');
                    };
                }

                $self.on('show.bs.modal', function() {
                    element.dispatchEvent(new Event('show.bs.modal', {"bubbles":true, "cancelable":false}))

                    // Comply with the Joomla API
                    // Set the current Modal ID
                    Joomla.Modal.setCurrent(element);

                    // @TODO throw the standard Joomla event
                    if ($self.data('url')) {
                        var modalBody = $self.find('.modal-body');
                        var el;
                        modalBody.find('iframe').remove();

                        // Hacks because com_associations and field modals use pure javascript in the url!
                        if ($self.data('iframe').indexOf("document.getElementById") > 0){
                            var iframeTextArr = $self.data('iframe').split('+');
                            var idFieldArr = iframeTextArr[1].split('"');

                            if (!document.getElementById(idFieldArr[1])) {
                                el = eval(idFieldArr[0]);
                            } else {
                                el = document.getElementById(idFieldArr[1]).value;
                            }

                            var data_iframe = iframeTextArr[0] + el + iframeTextArr[2];
                            modalBody.prepend(data_iframe);
                        } else {
                            modalBody.prepend($self.data('iframe'));
                        }
                    }
                }).on('shown.bs.modal', function() {
                    element.dispatchEvent(new Event('shown.bs.modal', {"bubbles":true, "cancelable":false}))

                    var modalHeight = $('div.modal:visible').outerHeight(true),
                        modalHeaderHeight = $('div.modal-header:visible').outerHeight(true),
                        modalBodyHeightOuter = $('div.modal-body:visible').outerHeight(true),
                        modalBodyHeight = $('div.modal-body:visible').height(),
                        modalFooterHeight = $('div.modal-footer:visible').outerHeight(true),
                        padding = $self.offsetTop,
                        maxModalHeight = ($(window).height()-(padding*2)),
                        modalBodyPadding = (modalBodyHeightOuter-modalBodyHeight),
                        maxModalBodyHeight = maxModalHeight-(modalHeaderHeight+modalFooterHeight+modalBodyPadding);
                    if ($self.data('url')) {
                        var iframeHeight = $('.iframe').height();
                        if (iframeHeight > maxModalBodyHeight){
                            $('.modal-body').css({'max-height': maxModalBodyHeight, 'overflow-y': 'auto'});
                            $('.iframe').css('max-height', maxModalBodyHeight-modalBodyPadding);
                        }
                    }
                    // @TODO throw the standard Joomla event
                }).on('hide.bs.modal', function() {
                    element.dispatchEvent(new Event('hide.bs.modal', {"bubbles":true, "cancelable":false}))
                    $('.modal-body').css({'max-height': 'initial', 'overflow-y': 'initial'});
                    $('.modalTooltip').tooltip('dispose');
                    // @TODO throw the standard Joomla event
                }).on('hidden.bs.modal', function() {
                    element.dispatchEvent(new Event('hidden.bs.modal', {"bubbles":true, "cancelable":false}))
                    // Comply with the Joomla API
                    // Remove the current Modal ID
                    Joomla.Modal.setCurrent('');
                    // @TODO throw the standard Joomla event
                });
            });
        }

There you have it

@Fedik
Copy link
Member

Fedik commented Nov 14, 2018

:neckbeard:

image

@Fedik
Copy link
Member

Fedik commented Nov 17, 2018

ah okay, I got it,
Bootstrap does not trigger Native events but jQuery events, and it can work only with jQuery,
that is 🐮 💩

@laoneo your fix is fine, sorry for confusing

@Fedik
Copy link
Member

Fedik commented Nov 17, 2018

I have tested this item ✅ successfully on d983f5e


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

@dgrammatiko
Copy link
Contributor

@Fedik why not use the code provided here and go js native?

@Fedik
Copy link
Member

Fedik commented Nov 17, 2018

@dgrammatiko yeah, it provide, but I do not see much sense in it,
while we use bootstrap.js we will be need jquery dependency anyway.

And if we do that hack, it will be harder to remove at later time, because all events will be namespaced to bootstrap 'xx.bs.modal'

@dgrammatiko
Copy link
Contributor

@Fedik check the comments (were there since the introduction of the modal API):

 // @TODO throw the standard Joomla event

I should have done that back then I guess

@Fedik
Copy link
Member

Fedik commented Nov 17, 2018

@dgrammatiko that also can be done at any time, when this modal will be replaced (if it happen) 😃

btw, @laoneo also have a @TODO, so all fine for now 😉

not a critical issue

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 2b029e0


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

@infograf768
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 20, 2018
@wilsonge wilsonge merged commit 0f0ec38 into joomla:4.0-dev Dec 13, 2018
@wilsonge wilsonge deleted the j4/fix/menu/reload branch December 13, 2018 18:07
@wilsonge
Copy link
Contributor

Thanks guys

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 13, 2018
@wilsonge wilsonge added this to the Joomla 4.0 milestone Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants