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

Adds action to store media using spatie/laravel-medialibrary #424

Open
wants to merge 7 commits into
base: 3.x
Choose a base branch
from

Conversation

core45
Copy link

@core45 core45 commented Jul 3, 2024

Purpose

By default, the media files are stored in the public disk in the media directory.

This adds the possibility to store media using spatie/laravel-medialibrary package.
The user will need to change the media_action key in the config file filament-tiptap-editor.php to SpatieMediaLibraryAction:

Copy link
Owner

@awcodes awcodes left a comment

Choose a reason for hiding this comment

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

Overall, how are the fields like alt, etc getting saved or read from spatie's package? Since it doesn't support those as native fields on the record?

$model = $component->getContainer()->model;
$media = $component->shouldPreserveFilenames()
? $model->addMedia($file)->toMediaCollection('tiptap-media')
: $model->addMedia($file)->usingFileName(Str::uuid())->toMediaCollection('ck-media');
Copy link
Owner

Choose a reason for hiding this comment

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

What is 'ck-media'?

@core45
Copy link
Author

core45 commented Jul 5, 2024

Good points. Thank you.
I'll correct the ck-media thing and add alt etc

@core45
Copy link
Author

core45 commented Jul 6, 2024

I fixed the ck-media mistake.

Concerning the alt, title and dimensions there is no need to implement anything using spatie/laravel-medialibrary because those parameters are stored in the text itself as the attributes eg. like this:

<img src="/storage/11/conversions/29e032e5-06b5-43cf-867c-80f33809801d-large.webp" alt="Alt Text" title="The photo title" width="1280" height="853" loading="lazy">

Is this OK?

@awcodes
Copy link
Owner

awcodes commented Jul 7, 2024

Wondering if this needs to be in the plugin itself? Seems like if could just be a Class in the app, since it can still be changed via the config.

@core45
Copy link
Author

core45 commented Jul 8, 2024

Well, you are the boss. But I think not. At lease no more than the "standard" way of storing files. I think adding this functionality do the core of the plugin will be beneficial taking under consideration how popular spatie/laravel-medialibrary is.
The very big downside to the standard way of storing files is that there is no control over the files (eg. converting/optimising them) and even worse they are not deleted when the parent record is deleted.
I know it is possible to use your Curator plug-in but I can't do it because I already use spatie's one and changing this would be a nightmare. I believe this can be true for many many others.

@awcodes
Copy link
Owner

awcodes commented Jul 8, 2024

Valid points.

How would you handle inserting the converted images back into the editor? i.e, upload creates a 'medium' sized image, where would that option come to insert that size as the image in the editor. And also what if it takes a while to generate that image, you would then need a placeholder to show in the editor, which would require updates to the JS extension internally.

Not totally opposed to including this, I just think there's more to it that would require some further troubleshooting, or maybe even a better implementation of something like a 'Media Provider' class or similar.

@core45
Copy link
Author

core45 commented Jul 10, 2024

You are right - this can be a problem as everybody can name the conversion as they see fit...

The 'large' or 'medium' comes from the model.

For example:

 public function registerMediaConversions(Media $media = null): void
    {
        $this->addMediaConversion('thumb')->fit(Fit::Crop, 50, 50)->optimize()->format('webp');
        $this->addMediaConversion('preview')->fit(Fit::Crop, 450, 300)->optimize()->format('webp');
        $this->addMediaConversion('large')->fit(Fit::Crop, 850, 548)->optimize()->sharpen(10)->format('webp');
    }

public function getFeaturedImageAttribute()
    {
        $file = $this->getMedia('featured_image')->last();

        if ($file) {
            $file->url       = $file->getUrl();
            $file->thumbnail = $file->getUrl('thumb');
            $file->preview   = $file->getUrl('preview');
            $file->large     = $file->getUrl('large');
        }

        return $file;
    }

So using my spatie solution you should be able to somehow decide which conversion (if any) you want to use.

I think this should be set by adding something like this (so with every model could use different conversion):

TiptapEditor::make('main_text')->useSpatieConversion('large'),

Or add it in setting file to use in all models.

What do you think?

@awcodes
Copy link
Owner

awcodes commented Jul 10, 2024

Honestly, I think this is assuming a lot. But I do see the value, despite knowing, at least right now, the best way forward. Like I'm torn on supporting something I don't personally use, but at the same time it's kind of the point of being able to provide your own custom Media class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants