dmitrybubyakin / nova-medialibrary-field

Laravel Nova field for managing the Spatie media library
MIT License
262 stars 62 forks source link

Nova 4 support #160

Closed theimerj closed 2 years ago

theimerj commented 2 years ago

Nova 4 support

Hello everyone, here is a PR for Nova 4 support we worked on with @kreejzak.

If you want to help with testing

In your composer.json

"require": {
    "dmitrybubyakin/nova-medialibrary-field": "dev-nova-4",
},
"repositories": [
    {
        "type": "git",
        "url": "https://github.com/dystcz/nova-medialibrary-field"
    },
],

Then run composer update

Good stuff

Still needs to be done

Possible enhancements


Thanks for the kickoff commits @aemaddin

dmitrybubyakin commented 2 years ago

@theimerj Wow, that's a huge amount of work! I'm not able to test it now, so let me know when this should be merged. Thanks for supporting this package!

theimerj commented 2 years ago

@theimerj Wow, that's a huge amount of work! I'm not able to test it now, so let me know when this should be merged. Thanks for supporting this package!

Hey @dmitrybubyakin, I will let you know when it reaches "mergeable status" haha. Right now, everyone can try it out and if issues arise, we will react to them.

It seems that I still have to fix nova composer installation for Github actions somehow.

Anyways, I will let you know. :)

theimerj commented 2 years ago

Hello @dmitrybubyakin, I fixed the tests, they are now running green here: https://github.com/dystcz/nova-medialibrary-field/tree/nova-4

However, I will need your help, because I had to add my Nova licence to repository secrets. Here is the commit: https://github.com/dystcz/nova-medialibrary-field/commit/7867fa6f0ceb93eb2f059d33f96c250cb1188deb

I just pasted my auth.json to ${{ secrets.COMPOSER_AUTH }} variable and it works fine. Do you have a Nova 4 licence? I do not think that the actions will run without it. I can provide our unlimited team licence for this purpose, but I don't know wether it goes against the Nova licence or not. :/

We are getting close to mergeable state!

dmitrybubyakin commented 2 years ago

Hi @theimerj, I don't have Nova 4 license. I think, tests can be run manually for now.

ChristianPavilonis commented 2 years ago

I started using the fork for nova 4. It's working so far. Let me know if I could help in any other way.

theimerj commented 2 years ago

Yeah, it is probably a good idea to finish this PR. I will go through it till the end of the week and then I think we will be ready to merge if @dmitrybubyakin agrees. We also use it in production and have not come across a problem yet. It needs some more polishing in my opinion, I will see what I can do, but I am not a frontend dev, so no promises.

kreejzak commented 2 years ago

So, I went through all the current vue components and made few changes. Mostly just formatted them using prettier to format tailwind classes and removed some inconsistencies such as duplicated classes. Also I've updated tailwindcss to use the latest version. There was no breaking changes so this version bump will only help in future developement imho.

theimerj commented 2 years ago

Ok @dmitrybubyakin, if you want, you can merge and publish a new version, which will work with nova 4. @kreejzak went through it, it all seems fine for now. Thank you.

For anyone interested in making progress with this version: There are new ways how Nova 4 handles stuff, so basically if you code dive long enough, you will be able to find ways to improve and do stuff "Nova 4 way". We will open a new PR when we have more time to work on this.

dmitrybubyakin commented 2 years ago

Thanks a lot to everyone!

dmitrybubyakin commented 2 years ago

v4.0 is published now