-
Notifications
You must be signed in to change notification settings - Fork 11
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
PB-1046: Fallback to default icon set if not found #1097
Conversation
web-mapviewer Run #3589
Run Properties:
|
Project |
web-mapviewer
|
Run status |
Passed #3589
|
Run duration | 04m 01s |
Commit |
7dcdf4dae8: PB-1046: Update dispatcher to the one call it.
|
Committer | Ismail Sunni |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
21
|
Skipped |
0
|
Passing |
211
|
281e2d9
to
ecf241f
Compare
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.
Good just a minor issue to changes before merging.
iconSetName: iconArgs.set, | ||
}), | ||
], | ||
dispatcher: 'kmlUtils.js', |
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.
Wrong dispatcher use the one on top of the file
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.
But this one is dispatched from kmlUtils.js
as a callback. I follow the same approach as in https://github.com/geoadmin/web-mapviewer/blob/develop/src/modules/map/components/openlayers/OpenLayersKMLLayer.vue#L86-L112
or should I change that one too?
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.
Yes and no, the callback is defined outside of kmlUtils.js and the caller is the file that call the helper function in kmlUtils.js.
I think for clarity it might make more sense to keep the file that contains the definition as dispatcher. This dispatcher can be useful for debugging so you will first look in kmlUtils.js and it will be then hard to find the dispatch in this file.
9250c0e
to
7dcdf4d
Compare
Test link