Closed ngunyimacharia closed 9 months ago
@ahmedkandel have you had a chance to look into this PR?
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.
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.
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);
}
@ngunyimacharia any context you have that might explain the above?
Hey @tractorcow, thank you for the feedback. Will look into this sometime today and get back to you.
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.
Awesome work @ngunyimacharia . I'll continue testing and let you know how I get along.
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.
Some of the buttons on light theme still need tweaking.
Everything else is working perfectly, good job. 👍
Thank you for the feedback @tractorcow. Will get the PR updated early this week 🙇🏿
I've done some UI changes, maybe @ngunyimacharia has some to review them. https://github.com/ngunyimacharia/nova-s3-multipart-upload/pull/1
That looks really cool. Nice job. 👍
Thanks @ngunyimacharia @tractorcow @mykkode for your collaboration. Please let me know when this PR is ready for review and merging.
I just tried this on a fresh install of nova 4 and the field just won't show up. I don't see anything in the logs but I'll be digging in. If you guys have any ideas what would cause that, let me know.
Ha. Nevermind. I am so dumb. This is a resource tool, not a field 🤣
When are you merging this PR for Nova 4?
Hey @ahmedkandel, all the feedback has been integrated into the PR. Feel free to re-review. cc @tractorcow, @mykkode
Hi @ngunyimacharia, thanks for the updates 👍, i have added some comments can you check them?
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.
Great job @ngunyimacharia 👍
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
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.
Thanks
You're welcome 🙏🏿