Skip to content

Conversation

@dgrammatiko
Copy link
Contributor

Scope

Eliminate mootools usage and use bootstrap modal for previewing the images.
Also scripts and stylesheets were called all over the place, now they follow Joomla's best practices

B/C

As long as the templates use bootstrap (admin area) there shouldn’t be any problems. Any overrides NEED to be updated!

Performance

Should be a bit quicker without mootools eating some of the rendering time, downloading, http requests etc.

Testing

Apply the patch and see if everything still works correctly.
Report any problems or any ideas for improvement

Preview

screen shot 2015-09-05 at 9 43 15

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Sep 5, 2015
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wilsonge George this muted part together with the other lines above should be enough for the modal to open in the parent window. Still haven’t found a way to update the video source, need to walk through the API/code of the player

Copy link
Contributor

Choose a reason for hiding this comment

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

just do something like

jQuery(document).ready(function($){
    window.parent.document.updateUploader();
    $('.img-preview, .preview').each(function(index, value) {
        $(this).on('click', function(e) {
            window.parent.jQuery('#videoPreview.modal video.mejs-player')[0].player.setSrc($(this).attr('href'));
            window.parent.jQuery('#imagePreview').modal('show');
            return false;
        });
    });
});

@ghazal
Copy link
Contributor

ghazal commented Sep 6, 2015

Looks OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this string needs to go as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it’s needed for #7819

Copy link
Contributor

Choose a reason for hiding this comment

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

This is #7819. Assuming you mean for #7810 but I thought you'd removed all the code for that from this PR?

@wilsonge
Copy link
Contributor

wilsonge commented Sep 6, 2015

Tested successfully


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

@wilsonge
Copy link
Contributor

wilsonge commented Sep 6, 2015

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 6, 2015
@wilsonge wilsonge added this to the Joomla! 3.4.5 milestone Sep 6, 2015
@Kubik-Rubik
Copy link
Member

Thank you @DGT41! Merged with 7e130b6

@dgrammatiko dgrammatiko deleted the ____bootstrap.Com.Media.Modal branch September 28, 2015 21:36
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 2, 2015
@zero-24 zero-24 modified the milestones: Joomla! 3.4.6, Joomla! 3.5.0 Oct 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants