Skip to content
This repository has been archived by the owner on Oct 17, 2022. It is now read-only.

Nova 4 support (needs testing) #66

Merged
merged 14 commits into from
Jun 16, 2022
Merged

Conversation

tanerkay
Copy link

@tanerkay tanerkay commented May 23, 2022

Thanks @mikebronner for the package, has been of great use so far! I've tried to tackle the changes necessary for Nova 4.x, and hoping for some feedback and testing help to get this ready to go.

Changes necessary to support:

  • Nova 3.x -> 4.x
    • Vue 2 -> 3
      • vue2-leaflet -> @vue-leaflet/vue-leaflet
      • brought in vue2-leaflet-geosearch as a first party component as the vendor version only supports Vue 2.x

Other changes:

  • maintain support for Laravel 8.x
  • upgrade other outdated NPM packages

Issues:

  • I haven't tested this PR thoroughly nor deployed it in production anywhere. In particular, the Geosearch part has not been tested.
  • I'm also running a modified version of the field which uses a geospatial POINT field rather than a separate latitude and longitude field, but have tried to keep those changes separate from the PR.
  • In my experience, for some reason the npm runners would dump some extra files into the dist/ directory, e.g. node_modules_leaflet_dist_images_marker-icon-2x_png.js. These are files that would otherwise be loaded in through promises. My workaround was to import some of the files preemptively, but it would be great to understand the root cause.

The workaround was adding the following to the top of resources/js/field.js:

import 'leaflet/dist/leaflet-src.esm';
import 'leaflet/dist/images/marker-icon-2x.png';
import 'leaflet/dist/images/marker-icon.png';
import 'leaflet/dist/images/marker-shadow.png';

@tanerkay
Copy link
Author

Related issue: #64

@lgtm-com
Copy link

lgtm-com bot commented May 23, 2022

This pull request introduces 1 alert and fixes 1 when merging c21e128 into 8766907 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

fixed alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 23, 2022

This pull request fixes 1 alert when merging 3e8485b into 8766907 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@mziraki
Copy link

mziraki commented Jun 10, 2022

@mikebronner
hi, any update?

@mostafaznv
Copy link

I wanted to add an ability to draw and store polygons but I'm waiting to accept this PR

@mikebronner
Copy link
Owner

Hi sorry for the delay, guys. I haven't been using Nova anymore for a while. Let me merge this, but as I cannot test this myself, please let me know if it breaks anything, so I can revert it.

@mikebronner mikebronner merged commit 0f979aa into mikebronner:master Jun 16, 2022
@mikebronner
Copy link
Owner

I wanted to add an ability to draw and store polygons

@mostafaznv that is cool functionality, for sure, but it goes beyond the scope of what this field is meant for. I would recommend creating your own field for marking on a map. I just wanted to let you know before you started work on a PR for that.

@mziraki
Copy link

mziraki commented Jun 16, 2022

@mikebronner it does not work at all! the field does not show!

@mikebronner
Copy link
Owner

Thanks for the feedback, I'll revert it.

@mikebronner
Copy link
Owner

I'm open to another PR that introduces Nova 4 support.

@mostafaznv
Copy link

I'm open to another PR that introduces Nova 4 support.

I would do it in coming days.

@egerb
Copy link

egerb commented Jun 22, 2022

@mikebronner it does not work at all! the field does not show!
yeap, because of ncaught Error: Cannot find module 'laravel-nova'

@mostafaznv
Copy link

I've tried to fix it. but due to some SSR features in vue-leaflet (vue 3) plugin, It didn't work as I expected.

So I've decided to create a new package based on other map libraries with some new features.

If will be published soon.
After that, maybe I put some extra time to make good PR for this package too.

@egerb
Copy link

egerb commented Jun 23, 2022

I've tried to fix it. but due to some SSR features in vue-leaflet (vue 3) plugin, It didn't work as I expected.

So I've decided to create a new package based on other map libraries with some new features.

If will be published soon. After that, maybe I put some extra time to make good PR for this package too.

Could you share with it, if possible

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

Successfully merging this pull request may close these issues.

5 participants