Fix toolbar undefined #295
Conversation
sashadev-sky
added
bug
dependencies
Possible breaking change
Okay, so I remember having a conversation about this with Justin a while back where he suggested at some breaking changes between the two namespace and the resulting difference in API support. I guess this was due and should stop conflict with the draw lib! Looks good to me! Also, none of the users were using the draw library, right? Then why exactly this change? I suppose if they stick to the |
|
Okay just saw your comment at the corresponding issue, I guess the reason for this was the leaflet-toolbar lib shifting the namespace to L.Toolbar2 form LeafletToolbar. Is that right, @sashadev-sky? |
|
@rexagod not positive. I looked through some issues on the page and I think I remember seeing that the old namespace was also not ES6 compatible (didn't let you import). Which had nothing to do with leaflet-draw-toolbar anyway the question now is what should we do? Do we ask the users to test out making this change. Or can they not really do that and we just need to announce a breaking change bump a major version and hope it fixes it? we could add leaflet toolbar locked in as 0.3.0 alternatively to a dependency (not dev), but as I said its not ES6 compatible which seems to be what most users are using. So I would vote for the former |
|
I vote for making the library ES6 compatible.
…On Wed, Jun 5, 2019 at 11:31 AM Sasha Boginsky ***@***.***> wrote:
@rexagod <https://github.com/rexagod> not positive. I looked through some
issues on the page and I think I remember seeing that the old namespace was
also not ES6 compatible (didn't let you import). Which had nothing to do
with leaflet-draw-toolbar
anyway the question now is what should we do? Do we ask the users to test
out making this change. Or can they not really do that and we just need to
announce a breaking change bump a major version and hope it fixes it?
we could add leaflet toolbar locked in as 0.3.0 alternatively to a
dependency (not dev), but as I said its not ES6 compatible which seems to
be what most users are using. So I would vote for the former
@jywarren <https://github.com/jywarren>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#295?email_source=notifications&email_token=AABE3CFWJERKU25I32WMJODPY7L4ZA5CNFSM4HTRYB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXAC6OY#issuecomment-499134267>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABE3CGDLGKMWWJ7NVBIYGLPY7L4ZANCNFSM4HTRYB2Q>
.
--
"Local color. Soak it up"
Virginia Vidaura
http://www.merkwelt.com/people/stan/
|
|
The people have spoken |
|
I agree. @jywarren how should we proceed with this PR? |
|
How about we try to poll some potential users of this lib by looking at recent commenters (@whoisstan are you a downstream user of this library?) We could pin an issue with this question for visibility... i dunno. i just imagine it's going to be hard to get feedback from people and we don't want to be too disruptive. That said, there's only one downstream user besides MapKnitter that I'm aware of -- https://github.com/publiclab/Leaflet.DistortableImage/network/dependents |
|
Er, actually that's a second one! I was thinking of: |
|
@jywarren I think based on what I see in the original where it was decided to swap namespaces: Leaflet/Leaflet.toolbar#48 this update will be necessary for moving the library forward as we have 4+ people having reported the issue here and a complaint there resulting in it making this change. It makes sense to move a repo forward with the rest of the packages on Github (leaflet and leaflet-toolbar both adapting to ES6). Additionally could open the door for more contributors! And also shouldn't really be breaking right? Unless libraries are extending our extended classes like we |
|
OK, then I am +1!
…On Wed, Jun 5, 2019 at 6:23 PM Sasha Boginsky ***@***.***> wrote:
@jywarren <https://github.com/jywarren> I think based on what I see in
the original where it was decided to swap namespaces:
Leaflet/Leaflet.toolbar#48
<Leaflet/Leaflet.toolbar#48> this update will
be necessary for moving the library forward as we have 4+ people having
reported the issue here and a complaint there resulting in it making this
change. It makes sense to move a repo forward with the rest of the packages
on Github (leaflet and leaflet-toolbar both adapting to ES6). Additionally
could open the door for more contributors!
And also shouldn't really be breaking right? Unless libraries are
extending our extended classes like we
we're doing with leaflet-toolbar. The key here will just be documentation.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#295?email_source=notifications&email_token=AAAF6JZ54OVIG2P5MRXC4EDPZA4CHA5CNFSM4HTRYB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXBF7CI#issuecomment-499277705>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6J44M5EPTA7VCGPLYKTPZA4CHANCNFSM4HTRYB2Q>
.
|
|
@jywarren Ok cool! So do you think it'd be good to release this with a version bump since this issue has been around for a few months? Then we can document the update in the releases like we started doing. I was thinking to try all this first on my own fork, and confirm it doesn't cause any problems with mapknitter |
|
That sounds great. Thanks!
…On Wed, Jun 5, 2019, 7:46 PM Sasha Boginsky ***@***.***> wrote:
@jywarren <https://github.com/jywarren> Ok cool! So do you think it'd be
good to release this with a version bump since this issue has been around
for a few months? Then we can document the update in the releases like we
started doing.
I was thinking to try all this first on my own fork, and confirm it
doesn't cause any problems with mapknitter
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#295?email_source=notifications&email_token=AAAF6J34H6V6H7LYW34VTNTPZBF3TA5CNFSM4HTRYB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXBKPDY#issuecomment-499296143>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6JZ6DSZOMBCH2KNEE5DPZBF3TANCNFSM4HTRYB2Q>
.
|
|
@jywarren <https://github.com/jywarren> i been using the npm registry but i
certainly can pull from git for a while.
…On Wed, Jun 5, 2019 at 4:09 PM Jeffrey Warren ***@***.***> wrote:
How about we try to poll some potential users of this lib by looking at
recent commenters ***@***.*** <https://github.com/whoisstan> are you a
downstream user of this library?)
We could pin an issue with this question for visibility... i dunno. i just
imagine it's going to be hard to get feedback from people and we don't want
to be too disruptive. That said, there's only one downstream user besides
MapKnitter that I'm aware of --
https://github.com/publiclab/Leaflet.DistortableImage/network/dependents
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#295?email_source=notifications&email_token=AABE3CGJ7UAO4XR7GCDOU4TPZAMRHA5CNFSM4HTRYB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXA32FA#issuecomment-499236116>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABE3CA2GWCL3AZJSJVP73LPZAMRHANCNFSM4HTRYB2Q>
.
--
"Local color. Soak it up"
Virginia Vidaura
http://www.merkwelt.com/people/stan/
|
|
+1 from downstream. My react wrapper uses a hacked up dist version of Leaflet.DistortableImage as I had problems with the toolbar and needed to chop it. First impression of the diff is it would will make it possible to refactor as a proper dependency. |
|
@jywarren Ok it works - but it requires a few other updates. Bower can't find the updated package because it is only released on npm. So we will need to:
This will be a breaking a change for repositories using Bower only, so we would release a new major version. We're planning on deprecating bower support this week as part of #240 so these changes will put that in motion now! Developers (and jywarren & @rexagod ) - please chime in if you don't like anything written above because dependency management is newish to me. |
|
MapKnitter switched to yarn last week! 👍 |
|
closing this as it has been fixed in #314 I'll mention everyone on the original issue! Would like to make sure the fix actually fixed it. Note ES6 compatibility has not been addressed yet |
Fixes #273, #123 (<=== Add issue number here)
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
gruntIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!
===============================================
Steps to reproduce bug in our dev environment:
make the changes from the 1st commit:
Steps to fix the bug in our dev environment:
make the changes from the 2nd commit