Skip to content

Feat/issue #332 zoom buttons#86

Open
stellarjaysoftware wants to merge 7 commits intoGreenstand:mainfrom
stellarjaysoftware:feat/issue_#332_zoom_buttons
Open

Feat/issue #332 zoom buttons#86
stellarjaysoftware wants to merge 7 commits intoGreenstand:mainfrom
stellarjaysoftware:feat/issue_#332_zoom_buttons

Conversation

@stellarjaysoftware
Copy link

font-weight: 500;
}

.leaflet-control-zoom.leaflet-bar {
Copy link
Author

Choose a reason for hiding this comment

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

this removes a broken looking offset shadow that appears around the zoom buttons

@dadiorchen
Copy link
Collaborator

@pratikmdhr can you help review this PR?

Copy link
Collaborator

@pratikmdhr pratikmdhr left a comment

Choose a reason for hiding this comment

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

@stellarjaysoftware Nice work!! apart from the comment I've made, it all looks good!


this.map = this.L.map(mountTarget, mapOptions)
if (this.zoomControlPosition) {
this.map.zoomControl.setPosition(this.zoomControlPosition)
Copy link
Collaborator

Choose a reason for hiding this comment

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

when this map-core is mounted to the map-client, setPosition will throw an error for the case when the zoomControl is set to false in map-client and zoomControlPosition is passed in some truthy value. You could perhaps change the condition to this.zoomControl && this.zoomControlPosition on line156 to be on the safe side

setPositionError

Copy link
Author

Choose a reason for hiding this comment

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

Thanks Pratik. I've made the update.

@stellarjaysoftware
Copy link
Author

@pratikmdhr, should I check in the built dist/main.js file? Or will that get built during release?

@pratikmdhr
Copy link
Collaborator

@pratikmdhr, should I check in the built dist/main.js file? Or will that get built during release?

Thanks! @stellarjaysoftware .. i believe npm run pre-publish step has been added to build-release GitHub action recently, so we don't need to build it manually.. @dadiorchen could you please verify??

@dadiorchen
Copy link
Collaborator

@pratikmdhr you are right, actually, we will delete i #84

@pratikmdhr
Copy link
Collaborator

@pratikmdhr you are right, actually, we will delete i #84

@dadiorchen i removed the main.js here #95
This PR can be merged too

@dadiorchen
Copy link
Collaborator

@stellarjaysoftware sorry for the delay, was busy in another part of the org, can you please solve the conflicts then I will merge this PR, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments