Skip to content

Layer: refactor openPopup/openTooltip: use common function#6613

Merged
cherniavskii merged 2 commits intoLeaflet:masterfrom
johnd0e:refactor-open-popup-tooltip
Apr 30, 2019
Merged

Layer: refactor openPopup/openTooltip: use common function#6613
cherniavskii merged 2 commits intoLeaflet:masterfrom
johnd0e:refactor-open-popup-tooltip

Conversation

@johnd0e
Copy link
Copy Markdown
Collaborator

@johnd0e johnd0e commented Apr 19, 2019 •
edited
Loading

Layer: refactor openPopup/openTooltip: use common function DivOverlay._openOverlay.

This change is Reviewable

@johnd0e johnd0e mentioned this pull request Apr 19, 2019
Closed
@johnd0e
Copy link
Copy Markdown
Collaborator Author

johnd0e commented Apr 19, 2019

Perhaps better name function differently, like _initOverlay.

mourner
mourner approved these changes Apr 22, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Re naming, maybe _updateOverlay would reflect what the code is doing best?

@johnd0e
Copy link
Copy Markdown
Collaborator Author

johnd0e commented Apr 22, 2019

maybe _updateOverlay would reflect what the code is doing best?

There are already several methods prefixed with update, so we need reflect function specific somehow

Besides, I do not feel that update is right word, as this code may be invoked before tooltip opened. It is a sort of preparatory step: here we determine what layer will be set as source, and calculate target latlng.

So may be _preOpen?

@mourner
Copy link
Copy Markdown
Member

mourner commented Apr 23, 2019

It is a sort of preparatory step

_prepareOpen then? or _prepareOverlay

@johnd0e johnd0e force-pushed the refactor-open-popup-tooltip branch from e224d83 to 7debbc9 Compare April 23, 2019 12:24
@johnd0e
Copy link
Copy Markdown
Collaborator Author

johnd0e commented Apr 23, 2019

_prepareOpen then? or _prepareOverlay

Ok, PR updated.

I've added one more commit, please check if it's acceptable.

cherniavskii
cherniavskii approved these changes Apr 30, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@cherniavskii cherniavskii merged commit 1e0d43b into Leaflet:master Apr 30, 2019
@johnd0e johnd0e mentioned this pull request May 6, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Sep 22, 2019
Open
Schleuse pushed a commit to Schleuse/Leaflet that referenced this pull request Dec 3, 2019
@Schleuse
Layer: refactor openPopup/openTooltip: use common function (Leaflet#6613
69188e8 
)

* Layer: refactor openPopup/openTooltip: use common function DivOverlay._prepareOpen

* More clear error message when trying to attach popup/tooltip to layer
without getLatLng function
@snyk-bot snyk-bot mentioned this pull request Apr 15, 2020
Open
@johnd0e johnd0e mentioned this pull request Apr 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mourner mourner mourner approved these changes

@cherniavskii cherniavskii cherniavskii approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@johnd0e @mourner @cherniavskii