-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: added Mapbox component #255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two important issues that need to be addressed:
- Static import of whole library (while also importing dynamically)
- Use of "gmap-click" event
Rest is questions / ramblings / style suggestions / etc.
const initMap = async () => { | ||
if (!mapEl.value) return; | ||
if (!fields.accessToken.value) return; | ||
const mapboxgl = await import("mapbox-gl"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome to see it being dynamically imported, makes a huge performance difference. 🚀
Have you tried building the docs (Vitepress) to see how that goes, for this one and Google Maps? I think you don't need the SSR flag (because you're not rendering on mount, only after access token is entered), but just to confirm.
Not sure if you're aware, but the docs when built basically scan every component get the streamsync
export and collect the fields and events. I've had problems with memory consumption. These type of components that rely on external libraries were more prone to cause issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used only npm run docs:dev
and it was working fine but now when i ran docs:build
it failed because in quickstart-tutorial I had link to localhost.
To quickly solve it I added following line to vitepress config
ignoreDeadLinks: 'localhostLinks',
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add docs build to CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use Netlify for docs, I think it can be easily hooked up to Github to build automatically. We're also on a Pro plan so the build allowance is quite generous.
I'm not sure if you had other idea in mind with CI for docs, keen to hear.
width: 100%; | ||
height: 100%; | ||
} | ||
.CoreMapbox .mask { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mask
thing you came up with is great and you've been using it across components. It's also expected we'll have to use it in other ones. What do you think about reutilising this across all components? I think no component would break if it had a mask, but some would if they didn't.
Rather than CSS, I'd probably go for a layer with a v-if
that only appears if IsBeingEdited && !isSelected
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to my TODO list. Lets do it in separate PR.
The same as in google maps. In fact we could name events for gmap and mapbox the same because they have same payload.
For Mapbox I see potential in displaying GeoJSON this could be really nice feature but this needs more discussion.