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

Fixing Double-click of image in Media Browser goes to blank page #14076

Merged
merged 2 commits into from Oct 17, 2018
Merged

Fixing Double-click of image in Media Browser goes to blank page #14076

merged 2 commits into from Oct 17, 2018

Conversation

Jako
Copy link
Collaborator

@Jako Jako commented Sep 7, 2018

What does it do?

Make sure that MODx.Media.onSelect code is not executed by removing the code.

Why is it needed?

MODx.Media.onSelect has hidden this.config.animEl || null before, which means that MODx.Media is hidden accidentally. That way a double-click of an image in the Media Browser goes to blank page.

Related issue(s)/PR(s)

#14075

@jonleverrier
Copy link
Contributor

I've tested this PR and it works as described.

Copy link
Contributor

@JoshuaLuckers JoshuaLuckers left a comment

Choose a reason for hiding this comment

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

Can the unreachable code be removed?

@Jako
Copy link
Collaborator Author

Jako commented Sep 7, 2018

I really don’t know where it was used, so yes.

@@ -1117,6 +1117,7 @@ Ext.extend(MODx.Media, Ext.Container, {
}

,onSelect: function(data) {
return false;
// @todo make sure this is never used
console.log('MODx.Media#onSelect', data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess console.log call should be definitely removed.

@Jako
Copy link
Collaborator Author

Jako commented Sep 11, 2018

I have removed all not needed 'make sure this is never used' code and the logging in those methods. The only part where a double click in the media browser leads to an action still works (doubleclick a file during editing a file selecting template variable or a file insert in the RTE). I did not find another place where the double click is used.

@Jako Jako added type-frontend Issues related to UI/UX issues, mostly about styles and frontend implementations on JavaScript. bug The issue in the code or project, which should be addressed. labels Sep 11, 2018
@Jako Jako added this to the v2.7.0 milestone Sep 11, 2018
@Jako Jako added the pr/review-needed Pull request requires review and testing. label Sep 11, 2018
@gpsietzema
Copy link
Contributor

gpsietzema commented Oct 13, 2018

Works as expected. Tested in latest versions of firefox/chrome/safari and edge.

@gpsietzema gpsietzema added pr/ready-for-merging Pull request reviewed and tested and ready for merging. and removed pr/review-needed Pull request requires review and testing. labels Oct 13, 2018
@opengeek opengeek merged commit 5ab2ab2 into modxcms:2.x Oct 17, 2018
@Jako Jako deleted the dblclick-media-browser branch October 17, 2018 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue in the code or project, which should be addressed. pr/ready-for-merging Pull request reviewed and tested and ready for merging. type-frontend Issues related to UI/UX issues, mostly about styles and frontend implementations on JavaScript.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants