-
-
Notifications
You must be signed in to change notification settings - Fork 707
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
fix breaking changes to css classnames #203
Conversation
Bundle size report: Size Change: +862 B
ℹ️ View Details
|
f55bfe7
to
a4c1086
Compare
I'm waiting for CI to pass... |
I've got some things to fix first, I noticed that in some places I've not split the class list |
Sure thing! Once you think the changes are compete ping me and I'll review the entire change. |
a4c1086
to
b3bda1d
Compare
@HarelM seems you need to approve the workflows for the CI to run |
All good now @HarelM |
Cool, can you help in the preparation of the release of 1.15.2:
This way I'll be able to simply merge this and create a release right afterward. |
Hi Everyone, First i want to say thanks for all the hard work. I've been following and tweaking things to work for my map at wifidb.net . it is currently on 1.15.1 I was wondering about these css changes. its is my understanding that now these are being reverted and saved until the 2.0 version. would it bepossible to make a 2.0 branch that the breaking changes can start getting put into?, or a 1.x legacy branch for the old code to live on while the master branch moves on? it seems eventually these breaking changes should be made because the mix of mapbox and maplibre gets confusing. for example, the one addon i use was mapbox inspect and originaly it was kind of a pain to modify becuase some things needed to be replaced with mapboxgl, while others (css) had to remain mapbox. eventually i got it working, but the mix made it harder to migrate. in 1.15.1 it was simpler, because all the mapbox text got changed to maplibregl, so it made a lot more sense when searching and replacing I will say i understand the complaint, that this breaks plugins... if it wasnt for the namespace change and css change, it would be a drop in replacement. but even with reverting this css change, the namespace is different and still breaks plugins.really if your going to revert css, it only seems half done without reverting the namespace. without that it still isnt reverse compatible. for example, there is thread that talked about this with a (react?) plugin where they ask if the mapbox namespace could be kept (perhaps with two versions or one that accespt both namespaces). if we had something like that it would be nice keeping things compatible...but otherwise compatiblity has ready been broken. if we had a version that kept the mapbox namespace, keeping the css with that version would make sense...and it would be a real drop in replacement. right now just moving the css still leaves it incompatible. I will note i like the idea of having 2 versions,one in the mapbox namespace and one in maplibregl namespace, because keeping the namespace allow plugins to continue unmodied, which would be nice because changing plugins you dont own doesn't alway end well. i speak from experience with mapbox inspect, which i fixed by modifing a release, but had touble getting it working from their sourcecode. it would be nice if the namespace and css hadn't changed so my plugins continued to work...but at the same time I think it makes sense for the project to move away from mapbox... |
I would say that the first breaking change that will be merged to main will make it 2.x, either the removal of the css or removal of token or something else. It will happen soon I believe. |
Personally I agree that things should move on, and this reversion (1.15.2) is a step backward. I have already moved my code to expect maplibregl, so this new realease only hurts me. i am going to have to wait untill a new branch with the css changes come, because i really dont want to revert my mapbox inspect changes. My post is more about making a compatibility branch for the people who actually need a more mapbox release, like this release is for. i did go into the namespace based on what i have read here and experience, but thats more an aside and understanding where changes like this come from. |
For example, if we had branch from the 1.13 release, people with a intrest in keeping a compable release could make fixes or merge non breaking changes. Then then move master to 2.0, which is where it seems people want the breaking changes and actually do these name and css changes, which is where it seems we probably should be going anyway. |
@acalcutt regarding the namespace change, there is a simple workaround to make maplibre works with the larger ecosystem: that I've shared here: #83 (comment) // webpack.config.js
module.export = {
// ...
resolve: {
alias: {
'mapbox-gl': 'maplibre-gl'
}
}
} or with rollup // rollup.config.js
import alias from '@rollup/plugin-alias';
module.exports = {
// ...
plugins: [
alias({
entries: [
{ find: 'mapbox-gl', replacement: 'maplibre-gl' },
]
})
]
}; The classname added / toggled by maplibre however are not so easily fixed and only a wider adoption from the plugin maintainers / tool developer can support this. That's the reasoning behind this PR: making it possible to continue to support the wider ecosystem while preparing the shift to a more maplibre centric one. |
Launch Checklist
TL;DR:
This PR reverts the breaking change introduced in v15.1.0 by adopting a dual naming scheme for the CSS Classe name throughout the project.
Addresses concerns voiced in #83 & #185
Closes #199
Resolves visgl/react-map-gl#1513
maplibre-gl-js
changelog:<changelog></changelog>
This PR is aimed at making maplibre 1.* more backward compatible and making sure that developers working with it and other mapbox/maplibre related tools are not depending on other packages adopting the new naming scheme right now.