implemented open_popup and close_popup methods

[PROgram52bc]

#912

[martinRenou]

Thanks a lot! This looks great :)

I only have a minor question

[martinRenou]

There seems to be a duplication of information between the location property and the location argument of this method.

Should this method default to the location property?

def open_popup(self, location=None):
    self.send({'msg': 'open', 'location': location is None ? self.location : location})

And should we change the location property if it's provided?

def open_popup(self, location=None):
    if location is not None:
        self.location = location
    self.send({'msg': 'open', 'location': location is None ? self.location : location})

[martinRenou]

I realize I've been writing non-valid Python code in my comment ahah. Let's try again:

def open_popup(self, location=None):
    self.send({'msg': 'open', 'location': self.location if location is None else location})

And should we change the location property if it's provided?

def open_popup(self, location=None):
    if location is not None:
        self.location = location
    self.send({'msg': 'open', 'location': self.location if location is None else location})

[PROgram52bc]

Yes, that makes a lot of sense to me. I didn't notice the location attribute in the Popup class, and I think they should be synced. See 6e6b070.

[martinRenou]

flake8 is complaining in the CI test:

ipyleaflet/leaflet.py:506:1: E302 expected 2 blank lines, found 1

I think we can ignore the UI tests failure, it looks unrelated.

[PROgram52bc]

flake8 is complaining in the CI test:

ipyleaflet/leaflet.py:506:1: E302 expected 2 blank lines, found 1

I think we can ignore the UI tests failure, it looks unrelated.

Thanks! There is another failure in the Visual Regression test. Do you have any idea what is causing that test to fail?

[martinRenou]

Thanks! There is another failure in the Visual Regression test. Do you have any idea what is causing that test to fail?

It looks like the basemap has changed a bit. You can ignore that. I don't think it's related to your change.

[PROgram52bc]

Do you think it is a good idea to also have an is_open attribute on the Popup class as mentioned in #239? What are some possible ways to implement that?

[martinRenou]

You can probably listen to the popupopen, and popupclose events on the Map https://leafletjs.com/reference-1.3.4.html#map-popupopen

But then you will need to be able to map those events to the right popup widgets?

[PROgram52bc]

But then you will need to be able to map those events to the right popup widgets?

I guess there is a reference to the popup object in the popup event?

[martinRenou]

I guess there is a reference to the popup object in the popup event?

To the popup object indeed, but there should not be a reference to the popup widget

[PROgram52bc]

I guess there is a reference to the popup object in the popup event?

To the popup object indeed, but there should not be a reference to the popup widget

I see. I guess we can tackle that issue in a separate PR then.

Any more comments?

[martinRenou]

I see. I guess we can tackle that issue in a separate PR then.

Yes that sounds like a good plan :)

Any more comments?

Sorry I wasn't clearer about the flake8 issue. It is still not resolved. I will push a commit to your branch fixing that if you don't mind.

[PROgram52bc]

Sorry I wasn't clearer about the flake8 issue. It is still not resolved. I will push a commit to your branch fixing that if you don't mind.

Yeah sure, please feel free :)

[martinRenou]

Actually you fixed it! Sorry I was confused by the diff.

Let's wait for the CI to be green then I'll merge :)

Thanks a lot!