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

Laravel Nova 4 compatibility #26

Merged
merged 30 commits into from
Feb 8, 2024
Merged

Conversation

ngunyimacharia
Copy link
Contributor

  • Updates asset and compilation process
  • Updates logic with Laravel Nova core changes
  • Replaces usage of modal with native browser confirmation
  • Icon components replaced with SVGs

@ngunyimacharia
Copy link
Contributor Author

@ahmedkandel have you had a chance to look into this PR?

@ahmedkandel
Copy link
Owner

Thanks @ngunyimacharia for the PR, I had a quick look at the PR and have some comments as soon as possible will discuss them with you as currently i'm so busy but hope to get a chance within the next weeks.

@tractorcow
Copy link
Contributor

I've done a bit of initial testing, I had a few issues with some of the routes.

E.g. http://localhost:8000/nova-vendor/nova-s3-multipart-upload/media/1/key/files was 404ing, due to FilesController::init() not being able to pull out the upload panel.

@tractorcow
Copy link
Contributor

If I change init() to the below it works.

private function init($request)
    {
        $resource = $request->findResourceOrFail();

        $this->model = $resource->model();

        $fields = $resource->availableFields($request)
            ->map(
                fn($field) => $field instanceof ResourceToolElement
                    ? $field->assignedPanel
                    : $field
            );

        $this->tool = $fields
            ->whereInstanceOf(NovaS3MultipartUpload::class)
            ->firstWhere('attribute', $request->route('field'));

        abort_unless($this->tool, 404);
    }

@tractorcow
Copy link
Contributor

@ngunyimacharia any context you have that might explain the above?

@ngunyimacharia
Copy link
Contributor Author

Hey @tractorcow, thank you for the feedback. Will look into this sometime today and get back to you.

@ngunyimacharia
Copy link
Contributor Author

Hey @tractorcow , thank you for your feedback once again. I have reviewed and tested your fix locally. It works well as well as simplifies the code. I have added them to this PR to make the review process easier. Let me know if anything else comes up.

@tractorcow
Copy link
Contributor

Awesome work @ngunyimacharia . I'll continue testing and let you know how I get along.

@tractorcow
Copy link
Contributor

Since nova 4 has dark / light theme support, we might need to adjust some of the styles for supporting both. I've noticed the current styles only really supports light theme, but looks a bit odd on dark.

image

image

@tractorcow
Copy link
Contributor

Some of the buttons on light theme still need tweaking.
image

@tractorcow
Copy link
Contributor

Everything else is working perfectly, good job. 👍

@ngunyimacharia
Copy link
Contributor Author

Thank you for the feedback @tractorcow. Will get the PR updated early this week 🙇🏿

@mykkode
Copy link

mykkode commented Oct 10, 2022

I've done some UI changes, maybe @ngunyimacharia has some to review them.
ngunyimacharia#1

Screenshot 2022-10-10 174356
Screenshot 2022-10-10 174429
Screenshot 2022-10-10 174443
Screenshot 2022-10-10 174458
Screenshot 2022-10-10 174513
Screenshot 2022-10-10 174335

@flexgrip
Copy link

Ha. Nevermind. I am so dumb. This is a resource tool, not a field 🤣

@ahmadbaignbs
Copy link

When are you merging this PR for Nova 4?

@ngunyimacharia
Copy link
Contributor Author

Hey @ahmedkandel, all the feedback has been integrated into the PR. Feel free to re-review. cc @tractorcow, @mykkode

Copy link
Owner

@ahmedkandel ahmedkandel left a comment

Choose a reason for hiding this comment

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

Maybe resources/js/tool.js content needs to be replace with:

import Tool from './components/Tool'

Nova.booting((app, store) => {
  app.component('nova-s3-multipart-upload', Tool)
})

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
nova.mix.js Outdated Show resolved Hide resolved
resources/js/components/FileCard.vue Outdated Show resolved Hide resolved
@ahmedkandel
Copy link
Owner

Hi @ngunyimacharia, thanks for the updates 👍, i have added some comments can you check them?

@ngunyimacharia
Copy link
Contributor Author

Hey @ahmedkandel , thank you for your feedback. I've resolved some of the comments. Feel free to review and let me know if anything is still pending.

@ahmedkandel
Copy link
Owner

Great job @ngunyimacharia 👍

@ahmedkandel
Copy link
Owner

Once you apply this last comment, can you build and try to install the package on a clean laravel+nova instance to test its functionality? before we release v2.0.0 which adds support to Nova v4

@ngunyimacharia
Copy link
Contributor Author

Once you apply this last comment, can you build and try to install the package on a clean laravel+nova instance to test its functionality? before we release v2.0.0 which adds support to Nova v4

Which comment is pending? Can't see it from my end.

Copy link
Owner

@ahmedkandel ahmedkandel left a comment

Choose a reason for hiding this comment

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

Sorry i forgot to submit the review

resources/js/components/FileCard.vue Outdated Show resolved Hide resolved
Copy link
Owner

@ahmedkandel ahmedkandel left a comment

Choose a reason for hiding this comment

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

Thanks

@ahmedkandel ahmedkandel merged commit d401e37 into ahmedkandel:master Feb 8, 2024
@ngunyimacharia
Copy link
Contributor Author

Thanks

You're welcome 🙏🏿

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.

6 participants