-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Introduce width
and height
to Icon style
#14364
Introduce width
and height
to Icon style
#14364
Conversation
📦 Preview the website for this branch here: https://deploy-preview-14364--ol-site.netlify.app/. |
Nice work, probably an often requested feature. Should We should have a test that ensures that the image is effectively scaled in the aspect ratio, when only one of I'd favor the opinion from @ahocevar or @tschaub in terms of potential API cluttering. I am +1 on getting this in. (edit: removed |
The |
I tried to create tests for both of theses cases but unfortunately the used image has no dimensions (
Ok. You're right. I'll see who it currently behaves and adapt the assertion. |
@mike-000 I adapted the assertion and checked how the example behaves when used with |
I couldn't manage to add a test for the scale. But I added the scale to the example to verify everthing works as expected. But of course a test would be much better, as a broken example won't be detected by CI. If someone has an idea how to fix the issue with the undefined dimension I could add the regarding tests. I could imagine that the 'load' eventlistener in the constructor has some timing issues with the testsuite. But as stated I couldn't handle it. |
@KaiVolland What do you think about marcjansen@24e3a53 which uses Feel free to cherry-pick if you want. |
The image will not load until required by the renderer unless you force a load as in https://github.com/openlayers/openlayers/blob/main/test/browser/spec/ol/style/icon.test.js#L29-L40 |
This looks like a saner approach. Maybe @KaiVolland can pick and amend as suggested. That'd be great. |
I added some code and tests to ensure that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On documentation comment, otherwise looks great to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Thanks for the nice collaboration, everyone.
First of all, thanks all the developers for this excellent library! I'm seeing a bug with this commit. You can not clone the Style or the Icon (needed to get clicked features), since it will try to set both height and width, and the scale, thus giving an AssertionError when creating the new Icon.
IMHO it would be better if this assertion is removed, since the properties are in sync, so they shouldn't cause a conflict (or maybe the assert should check if you specify both, that they are in sync. |
Thanks for the report, @gmerciel. Can you open a dedicated issue describing the bug? If we want to keep the assertion as is, one option would be to update the clone method to not include the scale of height and width are defined. Whatever the fix, tests should be added to cover the case. |
Thanks @marcjansen! |
This introduces the
width
andheight
property to theIcon
style.It includes an example.
The code is highlighy influenced by https://codesandbox.io/s/icon-forked-uof9by?file=/main.js so kudos to @mike-000
addresses #13508