Skip to content
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 setStyle method for Circle class #6128

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

martinRenou
Copy link

The Circle class inherits from CircleMarker class, which has a setStyle method which sets the radius. There was no equivalent in Circle class (the setStyle method was the one from the Path class), so we couldn't set the Circle radius with the setStyle method

@IvanSanchez
Copy link
Member

Why is this needed, when there is already a setRadius() method?

@martinRenou
Copy link
Author

I'm currently working on https://github.com/jupyter-widgets/ipyleaflet, we use python models on the backend for defining Leaflet widgets on the frontend. Every time the model changes on the backend, we update the frontend with a simple setStyle call passing the options defined in the model. This works fine for each attribute of the Circle class (those defined in the Path class: color, opacity, fillColor...), except for the radius.

@SylvainCorlay
Copy link

SylvainCorlay commented Apr 9, 2018

(setStyle appears to take the options dictionary and radius is listed in the options for the circle marker.)

@IvanSanchez
Copy link
Member

IvanSanchez commented Apr 9, 2018

BTW, this could be a much cleaner change if you just replace setStyle: Path.prototype.setStyle, with setStyle: CircleMarker.prototype.setStyle,. Can't be done that way, because of this._mRadius

@IvanSanchez
Copy link
Member

On second thought, it seems that the behaviour of setStyle is an oversight dating back to 508a75f. I'm partial to accepting this, but I'll wait for other maintainers to chip in.

Maybe we need to add some docstrings to make it clearer that setStyle on both Circle and CircleMarker do set the radius too.

@mourner
Copy link
Member

mourner commented Apr 17, 2018

I don't think this is an oversight. Radius not being a part of Circle "style" is intentional, because unlike CircleMarker, it'a physical property of the circle, just like geographical location. It's not a visual thing.

@johnd0e
Copy link
Collaborator

johnd0e commented May 2, 2020

@mourner
Should we close this?

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

Successfully merging this pull request may close these issues.

None yet

7 participants