Implemented user media manager #262

jaspervriends · 2021-01-31T19:37:08Z

I've implemented a user media upload manager with the idea of stopping multi-uploads from the same file. The files were already attached to the user, so I did not change anything of that.

Changelog:

Added responsive media manager
Simplified upload button in the composer
Added media manager button to the composer
Added /fof/uploads endpoint
Updated /fof/upload endpoint output
Updated FileSerializer
Added File frontend model

Modal features:
The media manager is expandable minded. Other extensions could open the FileManagerModal modal component and attach custom params to customize their needs.

The file manager is responsive, has drag & drop support and automatically selects the uploaded files. After clicking the 'Select files' button, the files will be added with their bbcode to the composer.

onSelect: Optional, custom callback, default: adds bbcode to composer
multiSelect: Optional, allow multiple selections, default true
restrictFileType: Optional, restrict file selection to specific types, default null, supported values:
- [array]
- image
- audio
- video

This PR
Let's focus on the code and improvements/removals:

General Media Manager code
Media Management API attributes
I removed an unused callback from https://github.com/FriendsOfFlarum/upload/blob/master/js/src/forum/components/DragAndDrop.js#L95 and removed the fof-upload-dropping and this.isDropping.

Updated composer buttons

Automatically select uploaded files

Example: Restricted to file type `image`

Night mode

Mobile view

- Added responsive media manager - Added /fof/uploads endpoint - Updated /fof/upload endpoint output - Updated FileSerializer - Added File frontend model

imorland · 2021-01-31T20:33:47Z

This really is great work @jaspervriends thank you for your time in creating this feature! Might take a couple of days to go over this, please bear with us :)

clarkwinkelmann · 2021-01-31T21:01:26Z

This looks very nice! I probably also won't be able to review immediately, but I will try to get to it!

How are the image preview managed? Is it the full image? If so, does it load all of the images immediately or only as you scroll down? If a forum allows very large uploads, or someone just has many images, will it not bring the website down by loading all of them?

jaspervriends · 2021-01-31T22:27:08Z

Awesome, take your time :)

This looks very nice! I probably also won't be able to review immediately, but I will try to get to it!

How are the image preview managed? Is it the full image? If so, does it load all of the images immediately or only as you scroll down? If a forum allows very large uploads, or someone just has many images, will it not bring the website down by loading all of them?

The files are loaded in batches as usual (20 files each), so it will depend on the overal image size. Currently the images are just using the path of the image. As there are no thumbnails generated by this extention :(

So if someone has any suggestions, let me know. Maybe generate it on-the-fly on a specific endpoint? fof/uploads/{file-id}/preview? If so, would we cache that on the server, or just in the browser?

davwheat

I'm not in FoF, so I won't make this a proper review, but I just had a few suggestions to point out to do with styling.

resources/less/forum/fileList.less

davwheat · 2021-02-01T14:53:08Z

resources/less/forum/fileList.less

+        padding: 0px;
+        margin: 0 -7px;
+
+        display: flex;


Could display: grid be easier for this? Has good support across browsers (even IE, if ~~you're mad~~ you provide the older spec properties too).

display: grid; grid-template-columns: repeat(3, minmax(10px, 1fr));

...would generate a grid with 3 columns. I think it's clearer than the calcs being used at the moment. Plus you can then use gap to create the spacing between the items (supported in FF61+, Chrome 66+, Safari 12+, or even earlier if you also provide grid-gap).

Reasoning behind minmax(10px, 1fr) over 1fr

I considered the display: grid too, but I wasn't sure if it was used in Flarum anywhere else, so I went for the safe flexbox variant instead. I could change it if needed.

resources/less/forum/fileList.less

resources/locale/en.yml

Comma splicing Co-authored-by: David Wheatley <[email protected]>

luceos · 2021-02-02T09:30:49Z

I'd love to hear other peoples' opinion about the following. Jasper and I had a discussion about the upload button, previous commits had it replaced by the media manager. Right now there's (yet) another button. Screen space is scarce, especially in the editor.

My suggestion is to allow an admin setting to choose between (upload button, media manager button or both). And then render accordingly. The media manager adds another click to upload.

davwheat · 2021-02-02T09:35:17Z

I'm a fan of that. Maybe it's time to also replace the "Upload" text with a tooltip to match the other buttons in the editor toolbar?

src/Api/Controllers/ListUploadsController.php

src/Api/Controllers/UploadController.php

jaspervriends · 2021-02-02T09:52:13Z

@luceos I think an admin setting would be fine. What would be the default setting then? Both buttons visible? And optionally hide one of the buttons?

Maybe it's time to also replace the "Upload" text with a tooltip to match the other buttons in the editor toolbar?

Yup, I already gave the upload button a tooltip :)

jaspervriends · 2021-02-10T16:19:41Z

@luceos @davwheat As requested, I've made some changes and worked on the feedback provided.

I've added an option called Composer buttons to the admin dashboard:

Which results into:

Both buttons visible:
Upload button only:
Media manager button only:

If there is anything else, let me know!

imorland

I've been checking out this PR for a couple of days locally, and I must say it works very, very well.

There's a couple of features that I personally feel this needs, and I'd really appreciate the thoughts of others here:

Ability to remove an upload from the media manager
Ability to access the media manager from the user profile
Ability for moderators to access the media of others, with the possibility to remove inappropriate content

I'm quite undecided on if the above should delay merging, or could be follow up changes

resources/locale/en.yml

js/src/admin/components/UploadPage.js

js/src/forum/components/FileManagerModal.js

davwheat · 2021-02-10T20:50:19Z

js/src/admin/components/UploadPage.js

@@ -193,6 +201,17 @@ export default class UploadPage extends ExtensionPage {
                                m('.helpText', app.translator.trans('fof-upload.admin.help_texts.download_templates')),
                                this.templateOptionsDescriptions(),
                            ]),
+                            m('fieldset', [


Non-blocking, but we should probably change this stuff to JSX for better readability in another PR.

Agreed, but as you say, this should be a separate PR to this feature

davwheat · 2021-02-10T20:53:01Z

Not sure on others' opinions, but it might be worth considering display: grid again. Flarum doesn't use it elsewhere, but some other styles core and this ext use are incompatible with IE11, so using it wouldn't be a major issue, but it'd just make styling far easier.

Up to you at the end of the day -- both work fine!

jaspervriends · 2021-02-11T11:57:22Z

@imorland I've updated the PR again and added an uploads overview to the user profile page (I agree it is a good addition too). Also, I added a fof-upload.viewUserUploads to view uploads from other users for moderation purpose. By default the permission is set to moderators in the migration file.

Also, I already added the fof-upload.deleteUserUploads to the same migration file for future usage. There is no delete functionality yet, also, I am wondering what would be a good approach for this as there are multiple ways to upload files to different adapters.

When a user sees their own profile, the button label is My uploads, when viewing another profile (and having the rights) they'll see User uploads.

When you click a file in the user profile page, it will open the file URL.

imorland · 2021-02-11T12:23:40Z

Superb @jaspervriends - I'll take a closer look after work tonight!

In regards to deletion, I agree it's a tricky subject. My feeling is that instead of actually deleting the file from disk/bucket/whatever, which in turn could likely impact posts that have it embedded, we should instead 'delete from media manager'.

Perhaps this could be done by adding a column to the fof-upload-files table, null by default, to act as a flag as to if the file should be included in the media library?

imorland · 2021-03-08T22:56:46Z

Any thoughts on the above @jaspervriends ?

Ideally I'd like to get this merged before we start working on the beta 16 updates for this extension

imorland · 2021-03-13T09:33:42Z

I'll merge this now, @davwheat and I will add the 'delete from media manager' feature as described #262 (comment)

jaspervriends added 3 commits January 31, 2021 19:53

Implemented media manager

4abe303

- Added responsive media manager - Added /fof/uploads endpoint - Updated /fof/upload endpoint output - Updated FileSerializer - Added File frontend model

Linter

cb3fa38

Updated restricted file types

60906e5

imorland requested review from clarkwinkelmann and dsevillamartin January 31, 2021 20:34

davwheat reviewed Feb 1, 2021

View reviewed changes

jaspervriends and others added 2 commits February 1, 2021 16:27

Update resources/locale/en.yml

e66e838

Comma splicing Co-authored-by: David Wheatley <[email protected]>

Update resources/locale/en.yml

a3ef575

Comma splicing Co-authored-by: David Wheatley <[email protected]>

davwheat reviewed Feb 2, 2021

View reviewed changes

src/Api/Controllers/ListUploadsController.php Outdated Show resolved Hide resolved

davwheat reviewed Feb 2, 2021

View reviewed changes

src/Api/Controllers/UploadController.php Outdated Show resolved Hide resolved

jaspervriends mentioned this pull request Feb 3, 2021

Implement FoF Upload v17development/flarum-blog#5

Closed

jaspervriends added 2 commits February 10, 2021 16:26

Updated feedback

8c2d114

Added composer button visibility

679f516

imorland reviewed Feb 10, 2021

View reviewed changes

resources/locale/en.yml Outdated Show resolved Hide resolved

luceos approved these changes Feb 10, 2021

View reviewed changes

davwheat reviewed Feb 10, 2021

View reviewed changes

Added user uploads to profile page

e3f46ab

imorland merged commit 03bd491 into FriendsOfFlarum:master Mar 13, 2021

eddiewebb mentioned this pull request Apr 19, 2021

200 OK error in Flarum 0.1.0-beta.16 eddiewebb/flarum-gpx-preview#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented user media manager #262

Implemented user media manager #262

jaspervriends commented Jan 31, 2021

imorland commented Jan 31, 2021

clarkwinkelmann commented Jan 31, 2021

jaspervriends commented Jan 31, 2021

davwheat left a comment

davwheat Feb 1, 2021 •

edited

Loading

jaspervriends Feb 1, 2021 •

edited

Loading

luceos commented Feb 2, 2021

davwheat commented Feb 2, 2021

jaspervriends commented Feb 2, 2021 •

edited

Loading

jaspervriends commented Feb 10, 2021

imorland left a comment

davwheat Feb 10, 2021

imorland Feb 11, 2021

davwheat commented Feb 10, 2021 •

edited

Loading

jaspervriends commented Feb 11, 2021

imorland commented Feb 11, 2021

imorland commented Mar 8, 2021

imorland commented Mar 13, 2021

Implemented user media manager #262

Implemented user media manager #262

Conversation

jaspervriends commented Jan 31, 2021

Updated composer buttons

Automatically select uploaded files

Example: Restricted to file type image

Night mode

Mobile view

imorland commented Jan 31, 2021

clarkwinkelmann commented Jan 31, 2021

jaspervriends commented Jan 31, 2021

davwheat left a comment

Choose a reason for hiding this comment

davwheat Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

jaspervriends Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

luceos commented Feb 2, 2021

davwheat commented Feb 2, 2021

jaspervriends commented Feb 2, 2021 • edited Loading

jaspervriends commented Feb 10, 2021

imorland left a comment

Choose a reason for hiding this comment

davwheat Feb 10, 2021

Choose a reason for hiding this comment

imorland Feb 11, 2021

Choose a reason for hiding this comment

davwheat commented Feb 10, 2021 • edited Loading

jaspervriends commented Feb 11, 2021

imorland commented Feb 11, 2021

imorland commented Mar 8, 2021

imorland commented Mar 13, 2021

Example: Restricted to file type `image`

davwheat Feb 1, 2021 •

edited

Loading

jaspervriends Feb 1, 2021 •

edited

Loading

jaspervriends commented Feb 2, 2021 •

edited

Loading

davwheat commented Feb 10, 2021 •

edited

Loading