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

[New Feature] tinyMCE drag and drop images #7435

Merged
merged 3 commits into from Nov 2, 2015

Conversation

Projects
None yet
@dgrammatiko
Member

dgrammatiko commented Jul 13, 2015

As the title suggests: enable drag and drop

This is once again another of those @nikosdion's great ideas, as expressed in #JAB15.

The concept here is very simple: drag an image into the tinyMCE’s content area and the image should automatically be uploaded and then the correct tag should be inserted in the text area.

What’s in it?

There are some minor changes in the com_media file.json.php controller in order to incorporate the needed feedback (namely the relative url for the image) and also to make sure that the name is URL safe (no spaces allowed here).

The tinyMCE plugin introduces a new private function for building the script code required fro the functionality. That’s if the option is enabled in the back end (it is by default).
Also introduced an option for a custom solder (showable only if drag&drop is enabled) to save the images exists
The code also checks if the user is allowed to upload images.
There is a spinning joomla gif to indicate that the upload is actually happening
Also if the image gets successfully uploaded a green overlay will indicate the success
If an error happened the error will display shortly on a red background
For files dropped that are not images nothing will happen
For images that the filename already exists on the server the upload will not happen and the image that is already in the server will appear in the tinyMCE content area. (failsafe we can improve that by introducing incremental filenames as we already do for existing article aliases)

Preview

screenshot 2015-10-25 02 14 10

#### Testing

Apply the patch
Edit tinyMCE’s options to specify the folder for the images
Test it front end - back end

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Jul 13, 2015

Contributor

Cool idea 👍
From the first look I don't like that you put a specific "tiny" controller into com_media. I don't think it is a good idea to have code in com_media which is specific to an editor.
I would try to use the existing file.json controller.

If that doesn't work then I would suggest to use com_ajax and do the upload part as a AJAX plugin event in the editor plugin itself. This way the code for tiny isn't spread all over the place but contained all in the plugin. 😄
Since Joomla 3.4 or so the plugin doesn't need to be anymore in the ajax group. Thus it works for all plugins as long as the event name (function name) starts with onAjax.... It would also be a great way to showcase what is possible :)

Contributor

Bakual commented Jul 13, 2015

Cool idea 👍
From the first look I don't like that you put a specific "tiny" controller into com_media. I don't think it is a good idea to have code in com_media which is specific to an editor.
I would try to use the existing file.json controller.

If that doesn't work then I would suggest to use com_ajax and do the upload part as a AJAX plugin event in the editor plugin itself. This way the code for tiny isn't spread all over the place but contained all in the plugin. 😄
Since Joomla 3.4 or so the plugin doesn't need to be anymore in the ajax group. Thus it works for all plugins as long as the event name (function name) starts with onAjax.... It would also be a great way to showcase what is possible :)

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 13, 2015

Contributor
Contributor

brianteeman commented Jul 13, 2015

Show outdated Hide outdated administrator/components/com_media/controllers/tiny.php
// }
JFactory::getDocument()->setMimeEncoding( 'application/json' );
JResponse::setHeader('Content-Disposition','attachment;filename="progress-report-results.json"');

This comment has been minimized.

@mbabker

mbabker Jul 13, 2015

Member

Set this directly to the application please, JResponse is deprecated.

@mbabker

mbabker Jul 13, 2015

Member

Set this directly to the application please, JResponse is deprecated.

Show outdated Hide outdated administrator/components/com_media/controllers/tiny.php
if (!JSession::checkToken('request'))
{
$this->app->enqueueMessage(JText::_('JINVALID_TOKEN'), 'error');
return; //some json error text

This comment has been minimized.

@mbabker

mbabker Jul 13, 2015

Member

If this is always handling AJAX, you can do this:

echo new JResponseJson(new Exception(JText::_('JINVALID_TOKEN')));
$this->app->close();

(Closing the app just keeps plugins from screwing up the response)

@mbabker

mbabker Jul 13, 2015

Member

If this is always handling AJAX, you can do this:

echo new JResponseJson(new Exception(JText::_('JINVALID_TOKEN')));
$this->app->close();

(Closing the app just keeps plugins from screwing up the response)

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 13, 2015

Member

@Bakual i think I need to talk to @Buddhima and try to get one upload controller that fits all requests. The problem is that new media is not yet production ready!

@brianteeman They introduced a new Upload API but that works with a button next to the field source in the image pop up. try it in their at tiny

Member

dgrammatiko commented Jul 13, 2015

@Bakual i think I need to talk to @Buddhima and try to get one upload controller that fits all requests. The problem is that new media is not yet production ready!

@brianteeman They introduced a new Upload API but that works with a button next to the field source in the image pop up. try it in their at tiny

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 13, 2015

Contributor
Contributor

brianteeman commented Jul 13, 2015

Show outdated Hide outdated plugins/editors/tinymce/tinymce.php
// console.log(e);
// console.log(e.dataTransfer.files);
// console.log('the content '+ed.getContent());
// alert('dragdrop' + e.dataTransfer.files);

This comment has been minimized.

@mbabker

mbabker Jul 13, 2015

Member

Can we make sure all the debug code is pulled out before merging?

@mbabker

mbabker Jul 13, 2015

Member

Can we make sure all the debug code is pulled out before merging?

Show outdated Hide outdated plugins/editors/tinymce/tinymce.php
}
";
$path = 'banners';

This comment has been minimized.

@mbabker

mbabker Jul 13, 2015

Member

I assume this shouldn't be a hard reference?

@mbabker

mbabker Jul 13, 2015

Member

I assume this shouldn't be a hard reference?

This comment has been minimized.

@dgrammatiko

dgrammatiko Jul 13, 2015

Member

Of course not, but I am not so sure how to handle this. One more option in the tinyMCE configuration?
Also I added one more switch that will enable disable this feature (ok haven’t done the xml file yet) 😃

@dgrammatiko

dgrammatiko Jul 13, 2015

Member

Of course not, but I am not so sure how to handle this. One more option in the tinyMCE configuration?
Also I added one more switch that will enable disable this feature (ok haven’t done the xml file yet) 😃

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jul 13, 2015

Member

@brianteeman I think this PR is adding the code needed to turn it on and make the feature work server side (something's gotta process the images).

Member

mbabker commented Jul 13, 2015

@brianteeman I think this PR is adding the code needed to turn it on and make the feature work server side (something's gotta process the images).

@Fedik

This comment has been minimized.

Show comment
Hide comment
@Fedik

Fedik Jul 14, 2015

Contributor

@dgt41 👍 very cool! .. here is link for Tiny upload http://www.tinymce.com/wiki.php/Configuration:images_upload_url

I also think that file.upload will be better than tiny.upload ... potentially this can be used by other extensions 😉

Contributor

Fedik commented Jul 14, 2015

@dgt41 👍 very cool! .. here is link for Tiny upload http://www.tinymce.com/wiki.php/Configuration:images_upload_url

I also think that file.upload will be better than tiny.upload ... potentially this can be used by other extensions 😉

@zero-24 zero-24 added the New Feature label Jul 14, 2015

@dgrammatiko dgrammatiko changed the title from [New Feature] RFC tinyMCE drag an drop images to [New Feature] RFC tinyMCE drag and drop images Jul 14, 2015

@nikosdion

This comment has been minimized.

Show comment
Hide comment
@nikosdion

nikosdion Jul 14, 2015

Contributor

Where should you put the controller part?

Regarding the controller part, I'd say that it's perfectly acceptable leaving it in com_media. It is generic code which can be reused by other editors or extensions which want to implement asynchronous uploads. It can even be used by com_media itself to implement d'n'd uploads (hint!).

Implementation of the controller

Do you need to call the files array tinyFiles or can we use a more generic name, e.g. uploadedMediaFiles?

Do we really have to go through $_SERVER['CONTENT_LENGTH']? The PHP files arrays do give you the size of uploaded media. Do take a look at the existing helper method JHelperMedia::canUpload. Use it instead of custom code in lines 70:97. Centralised code can be kept secure more easily.

Also, is there a compelling reason for using the code in lines 131:146 instead of JFile::upload? The latter is much better because it allows us to centrally control the security model for uploads. Whenever possible go through the existing helper methods instead of inventing new ways. It's easier for managing the system security.

Contributor

nikosdion commented Jul 14, 2015

Where should you put the controller part?

Regarding the controller part, I'd say that it's perfectly acceptable leaving it in com_media. It is generic code which can be reused by other editors or extensions which want to implement asynchronous uploads. It can even be used by com_media itself to implement d'n'd uploads (hint!).

Implementation of the controller

Do you need to call the files array tinyFiles or can we use a more generic name, e.g. uploadedMediaFiles?

Do we really have to go through $_SERVER['CONTENT_LENGTH']? The PHP files arrays do give you the size of uploaded media. Do take a look at the existing helper method JHelperMedia::canUpload. Use it instead of custom code in lines 70:97. Centralised code can be kept secure more easily.

Also, is there a compelling reason for using the code in lines 131:146 instead of JFile::upload? The latter is much better because it allows us to centrally control the security model for uploads. Whenever possible go through the existing helper methods instead of inventing new ways. It's easier for managing the system security.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Jul 14, 2015

Contributor

Fully agree with the controller being in com_media when it's generic.
Currently it's named tiny.php which doesn't look generic to me. And we already have a file.json.php controller with an upload task. That's why I suggested to try and use that. It looks like an Ajax upload endpoint, but haven't checked.

Contributor

Bakual commented Jul 14, 2015

Fully agree with the controller being in com_media when it's generic.
Currently it's named tiny.php which doesn't look generic to me. And we already have a file.json.php controller with an upload task. That's why I suggested to try and use that. It looks like an Ajax upload endpoint, but haven't checked.

@nikosdion

This comment has been minimized.

Show comment
Hide comment
@nikosdion

nikosdion Jul 14, 2015

Contributor

OK, fair enough. Having a single JSON endpoint sounds like a good idea.

Contributor

nikosdion commented Jul 14, 2015

OK, fair enough. Having a single JSON endpoint sounds like a good idea.

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 14, 2015

Member

@Bakual @nikosdion we still need the controller to return the relative path and the file name of the file uploaded! Which is not available right now, right?

Member

dgrammatiko commented Jul 14, 2015

@Bakual @nikosdion we still need the controller to return the relative path and the file name of the file uploaded! Which is not available right now, right?

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Jul 14, 2015

Contributor

I think it should be possible to adjust/extend the existing controller to return that.

You can either add the additional information to the existing JSON response or in worst case add a URL parameter (eg &version=2) which acts as toggle for the response, keeping old behavior for B/C and returning new version of response if parameter is present.

Contributor

Bakual commented Jul 14, 2015

I think it should be possible to adjust/extend the existing controller to return that.

You can either add the additional information to the existing JSON response or in worst case add a URL parameter (eg &version=2) which acts as toggle for the response, keeping old behavior for B/C and returning new version of response if parameter is present.

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Jul 14, 2015

Member

( ! ) Fatal error: Can't use method return value in write context in ROOT/plugins/editors/tinymce/tinymce.php on line 1003

I had patched with 4.2.1 and then this one.

Member

infograf768 commented Jul 14, 2015

( ! ) Fatal error: Can't use method return value in write context in ROOT/plugins/editors/tinymce/tinymce.php on line 1003

I had patched with 4.2.1 and then this one.

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 14, 2015

Member

@infograf768 sorry about that JM, latest commit should fix that

Member

dgrammatiko commented Jul 14, 2015

@infograf768 sorry about that JM, latest commit should fix that

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Jul 14, 2015

Member

hmm. Patched with #7423, then patched with this.
Addedimagetools, in the plugin array line 265 of tinymce.php + changed the line in advanced mode to
plugins : \"table link image code hr charmap autolink lists importcss imagetools\",

Result is negative: I can't see the new icon in the image popup and drag and drop does not work here.

Maybe I missed something?

Member

infograf768 commented Jul 14, 2015

hmm. Patched with #7423, then patched with this.
Addedimagetools, in the plugin array line 265 of tinymce.php + changed the line in advanced mode to
plugins : \"table link image code hr charmap autolink lists importcss imagetools\",

Result is negative: I can't see the new icon in the image popup and drag and drop does not work here.

Maybe I missed something?

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Jul 14, 2015

Member

I also have
Timestamp: 14/07/15 18:37:30
Error: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data
Source File: http://localhost:8888/trunkgitnew/administrator/index.php?option=com_content&view=article&layout=edit&id=94
Line: 517

Member

infograf768 commented Jul 14, 2015

I also have
Timestamp: 14/07/15 18:37:30
Error: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data
Source File: http://localhost:8888/trunkgitnew/administrator/index.php?option=com_content&view=article&layout=edit&id=94
Line: 517

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Jul 14, 2015

Member

These are also the errors I have when displaying the popup

screen shot 2015-07-14 at 18 40 06

Member

infograf768 commented Jul 14, 2015

These are also the errors I have when displaying the popup

screen shot 2015-07-14 at 18 40 06

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 14, 2015

Member

@infograf768 this PR doesn’t require #7423, please try it with the earlier version 4.1.6 (?)

Member

dgrammatiko commented Jul 14, 2015

@infograf768 this PR doesn’t require #7423, please try it with the earlier version 4.1.6 (?)

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Jul 14, 2015

Member

You mean it does not require the new imagetools plugin? I am confused.

Member

infograf768 commented Jul 14, 2015

You mean it does not require the new imagetools plugin? I am confused.

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 14, 2015

Member

drag and drop is not part of the image tools, actually image tools are a bunch of plugins for client side image editing plus some generic upload code. So yes the drag and drop doesn’t really require the images tools

Member

dgrammatiko commented Jul 14, 2015

drag and drop is not part of the image tools, actually image tools are a bunch of plugins for client side image editing plus some generic upload code. So yes the drag and drop doesn’t really require the images tools

Show outdated Hide outdated administrator/components/com_media/controllers/file.json.php
if (!MediaHelper::canUpload($file, $err))

This comment has been minimized.

@dgrammatiko

dgrammatiko Jul 14, 2015

Member

MediaHelper::canUpload() is deprecated and also for some reason break the upload

@dgrammatiko

dgrammatiko Jul 14, 2015

Member

MediaHelper::canUpload() is deprecated and also for some reason break the upload

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 14, 2015

Member

@mbabker thanks Mike. I also made that function static cause it was giving a strict error

Member

dgrammatiko commented Jul 14, 2015

@mbabker thanks Mike. I also made that function static cause it was giving a strict error

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jul 14, 2015

Member

The class is designed to be all OOP without statics, so change it to this please:

$mediaHelper = new JHelperMedia;

if (!$mediaHelper->canUpload($file, 'com_media'))
...
`
Member

mbabker commented Jul 14, 2015

The class is designed to be all OOP without statics, so change it to this please:

$mediaHelper = new JHelperMedia;

if (!$mediaHelper->canUpload($file, 'com_media'))
...
`
@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 14, 2015

Member

@mbabker Thanks again! Done!

Member

dgrammatiko commented Jul 14, 2015

@mbabker Thanks again! Done!

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 15, 2015

Member

@Bakual Thomas can you review the changes in the controller file.json.php?

Member

dgrammatiko commented Jul 15, 2015

@Bakual Thomas can you review the changes in the controller file.json.php?

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Jul 15, 2015

Contributor

@dgt41 I don't have time today to do a proper review. What I wonder is if we really need the returnUrl parameter the way you did it. I think you could just always add it to the response without breaking anything. Since the existing status and error response properties remain unchanged. Returning additional data should be backward compatible as long as the existing data isn't changed.
Or do I miss something?

Contributor

Bakual commented Jul 15, 2015

@dgt41 I don't have time today to do a proper review. What I wonder is if we really need the returnUrl parameter the way you did it. I think you could just always add it to the response without breaking anything. Since the existing status and error response properties remain unchanged. Returning additional data should be backward compatible as long as the existing data isn't changed.
Or do I miss something?

@zero-24 zero-24 referenced this pull request Jul 15, 2015

Closed

Some Codestyle #15

@dgrammatiko dgrammatiko changed the title from [New Feature] RFC tinyMCE drag and drop images to [New Feature] tinyMCE drag and drop images Jul 16, 2015

@dgrammatiko dgrammatiko referenced this pull request Jul 16, 2015

Merged

Update tinyMCE 4.2.1 #7423

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 16, 2015

Member

I think I am done here! Tested and works well on Safari, Firefox, Opera and Chrome. Still haven’t tried on IE but I don’t see any reason why this will fail there (all the code is just some simple jquery stuff).
I would appreciate some feedback form @Bakual @mbabker @nikosdion so if everything is good from code review, people can start testing this

Member

dgrammatiko commented Jul 16, 2015

I think I am done here! Tested and works well on Safari, Firefox, Opera and Chrome. Still haven’t tried on IE but I don’t see any reason why this will fail there (all the code is just some simple jquery stuff).
I would appreciate some feedback form @Bakual @mbabker @nikosdion so if everything is good from code review, people can start testing this

@Bakual Bakual added this to the Joomla! 3.5.0 milestone Jul 16, 2015

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Jul 16, 2015

Contributor

Controller looks fine now. Thanks for the work you did 👍
Would be great to get some tests on this.

I've added the 3.5 milestone for now.

Contributor

Bakual commented Jul 16, 2015

Controller looks fine now. Thanks for the work you did 👍
Would be great to get some tests on this.

I've added the 3.5 milestone for now.

@peterlose

This comment has been minimized.

Show comment
Hide comment
@peterlose

peterlose Jul 16, 2015

Contributor

I can't get it to work in the backend. Nothing happens and no errors. Uploads fine in frontend, however the generated path seems wrong.

skaermbillede 2015-07-16 kl 22 28 51

skaermbillede 2015-07-16 kl 22 29 32

Contributor

peterlose commented Jul 16, 2015

I can't get it to work in the backend. Nothing happens and no errors. Uploads fine in frontend, however the generated path seems wrong.

skaermbillede 2015-07-16 kl 22 28 51

skaermbillede 2015-07-16 kl 22 29 32

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Oct 24, 2015

Member

@Andrej1966 About the first and third point: we are not dealing with our own structure here but with tinyMCE. Also you should try tinyMCE’s default behavior on drag and drop. The dashed line is somehow default for drag and drop check Facebook and Twitter, it’s not MY design it’s the default expected.
About trying to drop an image when the spinning logo is present: I guess I should look at this

Member

dgrammatiko commented Oct 24, 2015

@Andrej1966 About the first and third point: we are not dealing with our own structure here but with tinyMCE. Also you should try tinyMCE’s default behavior on drag and drop. The dashed line is somehow default for drag and drop check Facebook and Twitter, it’s not MY design it’s the default expected.
About trying to drop an image when the spinning logo is present: I guess I should look at this

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Oct 24, 2015

Member

And here is the styling of Facebook and twitter:

screenshot 2015-10-24 15 47 25
screenshot 2015-10-24 15 49 26

Member

dgrammatiko commented Oct 24, 2015

And here is the styling of Facebook and twitter:

screenshot 2015-10-24 15 47 25
screenshot 2015-10-24 15 49 26

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Oct 24, 2015

Member

Latest update: I changed the way that the spinner was injected (was with a div overlay, now directly in tinyMCE). This was breaking the functionality if the spinner was shown and another image was dropped in the editor, thanks @Andrej1966 for this.

Member

dgrammatiko commented Oct 24, 2015

Latest update: I changed the way that the spinner was injected (was with a div overlay, now directly in tinyMCE). This was breaking the functionality if the spinner was shown and another image was dropped in the editor, thanks @Andrej1966 for this.

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Oct 24, 2015

Member

And a preview of the Bootstrap progress bar for the file upload. Note this will not represent percentage (always 100%) for the moment but it is animated
screenshot 2015-10-25 02 14 10

Member

dgrammatiko commented Oct 24, 2015

And a preview of the Bootstrap progress bar for the file upload. Note this will not represent percentage (always 100%) for the moment but it is animated
screenshot 2015-10-25 02 14 10

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Oct 27, 2015

Member

@Andrej1966 I updated this so now it will have a progress bar

Member

dgrammatiko commented Oct 27, 2015

@Andrej1966 I updated this so now it will have a progress bar

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Oct 28, 2015

This PR has received new commits.

CC: @hdwebpros, @jessicadunbar, @jwaisner, @nikosdion, @peterlose


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

This PR has received new commits.

CC: @hdwebpros, @jessicadunbar, @jwaisner, @nikosdion, @peterlose


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

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Oct 28, 2015

Contributor

Can you guys please test so we can merge it into 3.5? Thanks.

Contributor

roland-d commented Oct 28, 2015

Can you guys please test so we can merge it into 3.5? Thanks.

@n9iels

This comment has been minimized.

Show comment
Hide comment
@n9iels

n9iels Oct 31, 2015

Contributor

Not sure if it is a problem, on my local test installation the image path will be like this:
path
Tested with MAMP, on windows 7.

If I try to upload a .pdf file via drag and drop it gives me the message Unable to upload file.: /joomla-cmsundefined. A message like "File type no supported" might be a better message?

No other problems found, I like it! 😄

Contributor

n9iels commented Oct 31, 2015

Not sure if it is a problem, on my local test installation the image path will be like this:
path
Tested with MAMP, on windows 7.

If I try to upload a .pdf file via drag and drop it gives me the message Unable to upload file.: /joomla-cmsundefined. A message like "File type no supported" might be a better message?

No other problems found, I like it! 😄

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Oct 31, 2015

Member

@n9iels about the path: does the image render in tinyMCE?
About the message: this PR was supposed to enhance the existing code of com_media. Obviously we can examine all the messages and maybe put new strings, but the initial idea was that com_media was going to be ready for 3.5.

Member

dgrammatiko commented Oct 31, 2015

@n9iels about the path: does the image render in tinyMCE?
About the message: this PR was supposed to enhance the existing code of com_media. Obviously we can examine all the messages and maybe put new strings, but the initial idea was that com_media was going to be ready for 3.5.

@n9iels

This comment has been minimized.

Show comment
Hide comment
@n9iels

n9iels Oct 31, 2015

Contributor

Yes the picture renders correctly and is visible.

I understand, maybe a good idea for a later pull request.
Since the image render correctly I think my test is successfull :)

Contributor

n9iels commented Oct 31, 2015

Yes the picture renders correctly and is visible.

I understand, maybe a good idea for a later pull request.
Since the image render correctly I think my test is successfull :)

@n9iels

This comment has been minimized.

Show comment
Hide comment
@n9iels

n9iels Oct 31, 2015

Contributor

I have tested this item successfully on 5ae9e86


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

Contributor

n9iels commented Oct 31, 2015

I have tested this item successfully on 5ae9e86


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

Show outdated Hide outdated administrator/components/com_media/controllers/file.json.php
'message' => JText::sprintf('COM_MEDIA_UPLOAD_COMPLETE', $returnUrl),
'error' => JText::sprintf('COM_MEDIA_UPLOAD_COMPLETE', $returnUrl),
'location' => $returnUrl
);

This comment has been minimized.

@dgrammatiko

dgrammatiko Oct 31, 2015

Member

@n9iels this simple change will make the directories look nice even for PCs, can you check it?
'location' => str_replace('\','/', $returnUrl)

@dgrammatiko

dgrammatiko Oct 31, 2015

Member

@n9iels this simple change will make the directories look nice even for PCs, can you check it?
'location' => str_replace('\','/', $returnUrl)

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment

This PR has received new commits.

CC: @hdwebpros, @jessicadunbar, @jwaisner, @n9iels, @nikosdion, @peterlose


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

dgrammatiko added some commits Oct 27, 2015

Restore client side check for image
and remove the restriction on the server side

Also replace windows paths backslashes with forwardslashes
@coolcat-creations

This comment has been minimized.

Show comment
Hide comment
@coolcat-creations

coolcat-creations Nov 1, 2015

Contributor

Hi there :)

  • German umlauts are just removed instead of replaced with for example Ö with oe, and Ü with ue, ß with ss, Ä with ae.
  • Successful with / and . inside the Name
  • I named a jpg .png and it was uploaded as .png - i don´t know if there is a possibility to see the real type of a file?
  • Can´t there be a rename dialog if file already exist?
  • by dragging an Illustrator File .ai - the border appears green but nothing happens then. Is it possible to color it red if filetype is not allowed?
  • Is it possible to add image processing (size, cropping,...) on this point, when the dragged picture is way too big?
Contributor

coolcat-creations commented Nov 1, 2015

Hi there :)

  • German umlauts are just removed instead of replaced with for example Ö with oe, and Ü with ue, ß with ss, Ä with ae.
  • Successful with / and . inside the Name
  • I named a jpg .png and it was uploaded as .png - i don´t know if there is a possibility to see the real type of a file?
  • Can´t there be a rename dialog if file already exist?
  • by dragging an Illustrator File .ai - the border appears green but nothing happens then. Is it possible to color it red if filetype is not allowed?
  • Is it possible to add image processing (size, cropping,...) on this point, when the dragged picture is way too big?
@n9iels

This comment has been minimized.

Show comment
Hide comment
@n9iels

n9iels Nov 1, 2015

Contributor

I have tested this item successfully on 30dde3e

Slashes are now replaced 👍


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

Contributor

n9iels commented Nov 1, 2015

I have tested this item successfully on 30dde3e

Slashes are now replaced 👍


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

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Nov 1, 2015

Member

Wow that’s a big list 😃

  • About the German umlauts: I am using the standard function that is used elsewhere in com_media. If people don’t mind I can mix some transliteration function there. But the idea is either you use this tinyMCE drag and drop or the com_media old upload button you should get the same results. So if I change it for tinyMCE I guess that needs to be updated also for the com_media upload button.
  • About extension rename, it doesn’t really matter as the file metadata is also taken into consideration
    Alt text
  • Rename dialog: This is asynchronous data transfer so it needs some consideration, probably a v2
  • About the red border: Yes it can be easily done IF we are dealing with one file. The problem is that we can have multiple files and this complicates it, so if 2 files are jpg (ok) and one is .ai (not ok) what color should we use? maybe use a blue or grey color like Facebook or twitter?
  • Yes, but it needs to be done through a plugin. But don’t worry I am gonna publish that soon (For free of course). That is the mid term solution, once we get the new com_media we will be able to do so much more…

This was really constructive, thanks @designbengel 👍

Member

dgrammatiko commented Nov 1, 2015

Wow that’s a big list 😃

  • About the German umlauts: I am using the standard function that is used elsewhere in com_media. If people don’t mind I can mix some transliteration function there. But the idea is either you use this tinyMCE drag and drop or the com_media old upload button you should get the same results. So if I change it for tinyMCE I guess that needs to be updated also for the com_media upload button.
  • About extension rename, it doesn’t really matter as the file metadata is also taken into consideration
    Alt text
  • Rename dialog: This is asynchronous data transfer so it needs some consideration, probably a v2
  • About the red border: Yes it can be easily done IF we are dealing with one file. The problem is that we can have multiple files and this complicates it, so if 2 files are jpg (ok) and one is .ai (not ok) what color should we use? maybe use a blue or grey color like Facebook or twitter?
  • Yes, but it needs to be done through a plugin. But don’t worry I am gonna publish that soon (For free of course). That is the mid term solution, once we get the new com_media we will be able to do so much more…

This was really constructive, thanks @designbengel 👍

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Nov 1, 2015

Contributor

About the German umlauts: I am using the standard function that is used elsewhere in com_media. If people don’t mind I can mix some transliteration function there. But the idea is either you use this tinyMCE drag and drop or the com_media old upload button you should get the same results. So if I change it for tinyMCE I guess that needs to be updated also for the com_media upload button.

If this is the current behaviour I wouldn't change it here and call it out-of-scope and should be dealt with in another PR.

Keep up the good work guys 💯

Contributor

roland-d commented Nov 1, 2015

About the German umlauts: I am using the standard function that is used elsewhere in com_media. If people don’t mind I can mix some transliteration function there. But the idea is either you use this tinyMCE drag and drop or the com_media old upload button you should get the same results. So if I change it for tinyMCE I guess that needs to be updated also for the com_media upload button.

If this is the current behaviour I wouldn't change it here and call it out-of-scope and should be dealt with in another PR.

Keep up the good work guys 💯

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment

This PR has received new commits.

CC: @hdwebpros, @jessicadunbar, @jwaisner, @n9iels, @nikosdion, @peterlose


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

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Nov 1, 2015

Member

Just removed the green color, now the theme color of the toolbar will be used. So default will be light grey (same as twitter dropbox area 😃 )

Member

dgrammatiko commented Nov 1, 2015

Just removed the green color, now the theme color of the toolbar will be used. So default will be light grey (same as twitter dropbox area 😃 )

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Nov 1, 2015

Contributor

I have tested this item successfully on 3bb31c8

works great from a first look.


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

Contributor

zero-24 commented Nov 1, 2015

I have tested this item successfully on 3bb31c8

works great from a first look.


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

@n9iels

This comment has been minimized.

Show comment
Hide comment
@n9iels

n9iels Nov 1, 2015

Contributor

Looks better 👍

Contributor

n9iels commented Nov 1, 2015

Looks better 👍

@coolcat-creations

This comment has been minimized.

Show comment
Hide comment
@coolcat-creations

coolcat-creations Nov 1, 2015

Contributor

Thanks for the comments @dgt41 - the grey color is better i think because it does not make the user feel that the system will accept all the images.

There is an error message for duplicate file names, one for too large file size, but not at a wrong file type?

Regarding the umlauts i´d like to open a new issue because thats something really annoying in german when the umlauts are eliminated from the name @roland-d :-)

Can it be set this to change the "paste" position meanwhile the drag and drop process? Now i have to set the marker on the place where the image should be and then drag it anywhere.

Last but not least: it´s funny that after so much years of joomla i just realized that there are plugin parameters for tinymce :-) I´d rather search them in the "Global Configuration" and also expect some Permission Parameter Setting for the Editor - thats surely also another topic...

Contributor

coolcat-creations commented Nov 1, 2015

Thanks for the comments @dgt41 - the grey color is better i think because it does not make the user feel that the system will accept all the images.

There is an error message for duplicate file names, one for too large file size, but not at a wrong file type?

Regarding the umlauts i´d like to open a new issue because thats something really annoying in german when the umlauts are eliminated from the name @roland-d :-)

Can it be set this to change the "paste" position meanwhile the drag and drop process? Now i have to set the marker on the place where the image should be and then drag it anywhere.

Last but not least: it´s funny that after so much years of joomla i just realized that there are plugin parameters for tinymce :-) I´d rather search them in the "Global Configuration" and also expect some Permission Parameter Setting for the Editor - thats surely also another topic...

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Nov 1, 2015

Member

@designbengel starting from the last comment: there is a PR that will make tinyMCE ACL aware and is planned for 3.5 as well #8147
The paste position in fact is working but there is no blinking cursor and that makes it look like that is not working. Honestly the API of tinyMCE is not very helpful, trying to solve this. It also annoys me, as well!
About the wrong type: there isn’t one because the wrong file types are rejected in the client side and I followed the tinyMCE’s default behavior of silent rejection. I guess we could put another warning modal here…
I think we are getting closer to merging here 😃

Member

dgrammatiko commented Nov 1, 2015

@designbengel starting from the last comment: there is a PR that will make tinyMCE ACL aware and is planned for 3.5 as well #8147
The paste position in fact is working but there is no blinking cursor and that makes it look like that is not working. Honestly the API of tinyMCE is not very helpful, trying to solve this. It also annoys me, as well!
About the wrong type: there isn’t one because the wrong file types are rejected in the client side and I followed the tinyMCE’s default behavior of silent rejection. I guess we could put another warning modal here…
I think we are getting closer to merging here 😃

@coolcat-creations

This comment has been minimized.

Show comment
Hide comment
@coolcat-creations

coolcat-creations Nov 1, 2015

Contributor

:-) Great - one more suggestion for v2 - Here in github you have a "uploading your files..." text next to the progress icon.

Contributor

coolcat-creations commented Nov 1, 2015

:-) Great - one more suggestion for v2 - Here in github you have a "uploading your files..." text next to the progress icon.

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Nov 2, 2015

Contributor

@designbengel Can umlauts be used in filenames? They became part of a URL, I don't know if that is a problem. I understand it is an issue and if it can be improved we should but not in this PR though.

Can you share your final test results again? If all is OK, we have 2 successful commits and we can commit this gem.

Contributor

roland-d commented Nov 2, 2015

@designbengel Can umlauts be used in filenames? They became part of a URL, I don't know if that is a problem. I understand it is an issue and if it can be improved we should but not in this PR though.

Can you share your final test results again? If all is OK, we have 2 successful commits and we can commit this gem.

@nikosdion

This comment has been minimized.

Show comment
Hide comment
@nikosdion

nikosdion Nov 2, 2015

Contributor

@roland-d In theory any UTF-8 character can be used in a URL and supported by all modern browsers and servers. However, if you have any visitors still using IE7 or earlier (don't get me started...) they won't be able to access them. Some servers with questionable localisation settings may also mangle the names. So by necessity we transliterate all URLs to 7-bit ASCII and more specifically in the subset consisting of lowercase unaccented a-z, 0-9, dash, underscore and dot. It's very fun when you have Greek and can transliterate phonetically, by visual resemblance or according to the ISO standard... IMHO we should just silently let people have to use the ASCII subset as we do right now.

@test Still works for me :)

Contributor

nikosdion commented Nov 2, 2015

@roland-d In theory any UTF-8 character can be used in a URL and supported by all modern browsers and servers. However, if you have any visitors still using IE7 or earlier (don't get me started...) they won't be able to access them. Some servers with questionable localisation settings may also mangle the names. So by necessity we transliterate all URLs to 7-bit ASCII and more specifically in the subset consisting of lowercase unaccented a-z, 0-9, dash, underscore and dot. It's very fun when you have Greek and can transliterate phonetically, by visual resemblance or according to the ISO standard... IMHO we should just silently let people have to use the ASCII subset as we do right now.

@test Still works for me :)

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Nov 2, 2015

Contributor

Thanks lets go for it with 3.5 as first version 👍 -> RTC Thanks @dgt41 this is awesome work


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

Contributor

zero-24 commented Nov 2, 2015

Thanks lets go for it with 3.5 as first version 👍 -> RTC Thanks @dgt41 this is awesome work


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

@joomla-cms-bot joomla-cms-bot added the RTC label Nov 2, 2015

roland-d added a commit that referenced this pull request Nov 2, 2015

Merge pull request #7435 from dgt41/tinyMCE_dragANDdropIMG
[New Feature] tinyMCE drag and drop images

@roland-d roland-d merged commit 7fae6a0 into joomla:staging Nov 2, 2015

2 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@coolcat-creations

This comment has been minimized.

Show comment
Hide comment
@coolcat-creations

coolcat-creations Nov 2, 2015

Contributor

@roland-d no i didn´t meant the umlauts to stay in the name like they are. It´s usual to transform ü to ue, ö to oe, ä to ae and ß to ss - currently they are completely removed with is not good :-) so should i open a new request? :-) Fun example: Übergrößenträger -> "Uebergroeßentraeger" or "bergrentrger"

Edit: Oh s* sorry didn´t knew the RTC is removed by a comment - please add it again!

Contributor

coolcat-creations commented Nov 2, 2015

@roland-d no i didn´t meant the umlauts to stay in the name like they are. It´s usual to transform ü to ue, ö to oe, ä to ae and ß to ss - currently they are completely removed with is not good :-) so should i open a new request? :-) Fun example: Übergrößenträger -> "Uebergroeßentraeger" or "bergrentrger"

Edit: Oh s* sorry didn´t knew the RTC is removed by a comment - please add it again!

@joomla-cms-bot joomla-cms-bot removed the RTC label Nov 2, 2015

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Nov 2, 2015

Contributor

@designbengel Please create an issue for it, so we don't forget about it :) The RTC has been removed because this feature has been merged.

Contributor

roland-d commented Nov 2, 2015

@designbengel Please create an issue for it, so we don't forget about it :) The RTC has been removed because this feature has been merged.

@dgrammatiko dgrammatiko deleted the dgrammatiko:tinyMCE_dragANDdropIMG branch Nov 2, 2015

@lungkao

This comment has been minimized.

Show comment
Hide comment
@lungkao

lungkao Nov 13, 2015

folder for images upload /images only ? if have many picture not good

it possible upload to /images/2015/3/31/id ?

lungkao commented Nov 13, 2015

folder for images upload /images only ? if have many picture not good

it possible upload to /images/2015/3/31/id ?

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment