Skip to content

Conversation

@JenSeReal
Copy link
Contributor

@JenSeReal JenSeReal commented Aug 24, 2021

Summary of Changes

Refactoring of the Vue components for the Media Manager. Created components for repetitive code, so if you want to modify a button you don't have to change the button in 6 components. Also some logic got capsulated in components so the individual files have a reduced number of code. Everything should work as in previous commits. All this is done in preparation to later add dynamic editable file extensions and a small pdf edtior to edit pdfs in Joomla.

Testing Instructions

Just navigate to Content -> Media. Directories should have buttons to rename and delete, Videos, Audio, Documents and so on should have buttons to preview, rename, share, delete, download. JPG, JPEG and PNG should still be editable. Navigation with the arrow keys between the buttons should also still work

Actual result BEFORE applying this Pull Request

If you open the media manager you will see several items which are either directories, audio files, video files, image files, document files or if the file extension is unknown to joomla its just a file. If you hover over one item and click the toggle button (3 dots) you get to see the options what you can do with the file (preview, rename, share, delete, download, edit). The revealed buttons then perform the respective actions on click.

Expected result AFTER applying this Pull Request

The described functionality should still be the same.

Documentation Changes Required

No.

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.1-dev labels Aug 24, 2021
@laoneo
Copy link
Member

laoneo commented Aug 24, 2021

NICE, this was bugging me since the beginning. Next step would then be to offer plugins a way to add custom actions :-)

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Aug 24, 2021

Next step would then be to offer plugins a way to add custom actions

I was thinking the same as I was reviewing the code here. Also, it shouldn't be that hard but it requires some changes in the PHP world as described here: #34634 (comment)
Basically the supported extensions data struct should be something like

{
  supported: [
    {
      extension: 'png',
      mime: 'image/png',
      preview: true,
      upload: true,
      edit: true
    }
  ]
}

@laoneo
Copy link
Member

laoneo commented Aug 24, 2021

I was more referring to custom actions like "Send by Mail" or "Tweet picture" which can be added by plugins.

@@ -1,8 +1,10491 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this file. There have been no changes made to the package.json, so this file should not be modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the package-lock.json to to the stand of this commit. I kinda wonder why this has changed, because I used npm ci to build

@dgrammatiko
Copy link
Contributor

You still need some of the buttons to be able to act on specific media types or else the API will be an all or nothing. The media types object that I proposed above can be extended by the plugins to add the button name and enable it for whatever extension needs to be enabled (basically the plugin will have to register itself for whatever media type is supporting and append the vue js part before the mediamanager.js)

@laoneo
Copy link
Member

laoneo commented Aug 24, 2021

@dgrammatiko agree. But lets discuss this on another day to not disturb this pr.

@JenSeReal
Copy link
Contributor Author

@dgrammatiko do you think an intermediate solution would be to define extra fields, in installation/sql/mysql/base.sql with image_extensions_editable: "jpg,jpeg,png", then retrieving them in a template file and so make it visible to the client? With that one could get rid of the hardcoded editable file extensions for now.
A new data structure like you proposed it would make lots of sense in my eyes for the long-term but maybe this could be a short-term solution?

@dgrammatiko
Copy link
Contributor

With that one could get rid of the hardcoded editable file extensions for now.

We already do something similar, check #34634

A new data structure like you proposed it would make lots of sense in my eyes for the long-term but maybe this could be a short-term solution?

The short-time solution is already in place, what is needed (check my last comment in the other PR) is a way to have a pool of the mime/extentions and then a field in the media manager option where users could pick the extensions they want. Since we will provide the pool (similar to apache's or nginx mime.conf) we could hardcode there the buttons: preview, edit, etc and then the plugins would have only to append their name on the supported formats and register the .js file.

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Aug 24, 2021

@JenSeReal some feedback here, regarding the comment of @laoneo about custom buttons:

It would make sense to have the button components registered async ( https://vuejs.org/v2/guide/components-dynamic-async.html#Async-Components )like:

// These will be fetched from the mediamanager options rendered as json in the template layout
const buttons = [
{
  name: edit,
  url: '',
},
];

// The registration
for (button of buttons) {
  Vue.component(
    button.name,
    // A dynamic import returns a Promise.
    () => import(button.url)
  );
}

This way you have the foundation for any custom button

@laoneo
Copy link
Member

laoneo commented Aug 31, 2021

@bembelimen when you have a look at #35451, then you will see why this one here is needed.

@bembelimen
Copy link
Contributor

I'm just waiting for the 2 tests to merge ;)

@laoneo
Copy link
Member

laoneo commented Aug 31, 2021

@bembelimen I would trust more the system tests than the human tests 😄

@bembelimen
Copy link
Contributor

@dgrammatiko do you give a 👍 for this PR to merge or do you think we should update with your suggestion first? (probably you could then deliver some code, which we can merge with this PR?)

@dgrammatiko
Copy link
Contributor

@bembelimen my suggestions can go later, so this one it's fine as is.

Probably it would be a good idea to open a project in the GitHub to list the things that should be fixed/improved, get the discussion there and leave the PRs for the code part

@bembelimen bembelimen added this to the Joomla 4.1 milestone Sep 23, 2021
@bembelimen
Copy link
Contributor

Thx, then we just need some approved tests and we're good to go.

@bembelimen
Copy link
Contributor

Anyone with some successful tests? Would really like to merge it.

@richard67
Copy link
Member

We have 6 conflicting vue files. Maybe @dgrammatiko can help to solve the conflicts? I have no knowledge in vue.

@dgrammatiko
Copy link
Contributor

@richard67 I don't have sufficient permissions to do anything here, maybe @laoneo could fix the conflict if @JenSeReal can't do it

@richard67
Copy link
Member

Closing in favour of #35887 . Thanks @JenSeReal for your contribution.

@richard67 richard67 closed this Oct 24, 2021
@richard67
Copy link
Member

P.S.: The new PR still contains the commit history of this one here, so @JenSeReal will be at least mentioned as co-author of the new PR when it gets merged.

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.

8 participants