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

Add image overlay option #84

Merged
merged 5 commits into from
Nov 22, 2020
Merged

Add image overlay option #84

merged 5 commits into from
Nov 22, 2020

Conversation

jreyesr
Copy link

@jreyesr jreyesr commented Nov 21, 2020

(This is my first contribution to the project, so my apologies if I have overlooked anything obvious)

This PR adds support for an optional "image overlay" to be shown over the basemap and below the data. It uses the ImageOverlay function of Leaflet, see here for an example.

I needed this functionality to show interpolated data from a wireless sensor network, but it could also be used, for example, to show a more updated map (perhaps from a drone flight?) over the (potentially old or low-res) basemap.

I am still writing the tests. I would welcome guidance as to potential improvements, bugs or suggested changes. Documentation is also missing (I imagine that it would be necessary to update the README to explain the new config options).

@amotl
Copy link

amotl commented Nov 21, 2020

Dear @jreyesr,

thanks a bunch, we appreciate your contribution. The patch looks excellent, I will just address a minor nit.

I am still writing the tests.

Having tests which cover the new feature indeed would be nice!

Documentation is also missing.

Adding some words about the new feature would also be nice. Thanks for your prudence.

With kind regards,
Andreas.

P.S.: Please note that I've also just merged #81, so you might want to rebase on top of current master.

src/worldmap_ctrl.ts Outdated Show resolved Hide resolved
@jreyesr
Copy link
Author

jreyesr commented Nov 21, 2020

I've added some tests, an explanation of the settings in the README, and fixed the variable names as requested.

I'm still unsure of how to handle an image added to the README. The URL is currently relative to the README, since https://raw.githubusercontent.com/panodata/grafana-map-panel/master/src/images/overlay_example.png doesn't exist yet, but that will probably cause problems if the README is used on other webpages, such as the Grafana plugin page. Should I include an invalid URL anyway, and wait until the merge so that it becomes valid?

@amotl
Copy link

amotl commented Nov 22, 2020

I've added some tests, an explanation of the settings in the README, and fixed the variable names as requested.

Thank you so much!

I'm still unsure of how to handle an image added to the README. Should I include an invalid URL anyway, and wait until the merge so that it becomes valid?

Exactly.

So, I believe this would be ready to merge? Thanks again, your contribution went along pretty awesome.

@jreyesr jreyesr marked this pull request as ready for review November 22, 2020 17:21
@jreyesr
Copy link
Author

jreyesr commented Nov 22, 2020

Done. The link should now work, once the PR gets merged.

So, I believe this would be ready to merge?

Yes, as far as I can tell. I've just marked the PR as ready for review.

Thanks again

Of course! Happy to help.

@amotl amotl merged commit 2ce6f41 into grafana-toolbox:develop Nov 22, 2020
@amotl
Copy link

amotl commented Nov 22, 2020

Thanks a bunch. We just released grafana-map-panel 0.14.0 which includes your improvement.

@falconhome
Copy link

Hi Guys!
This overlay function is very useful, thanks a lot! I think my request is a "feature".
It is possible to add "Reload image on dasboard refresh" checkbox under the Custom image overlay settings?

Now the overlay image load when the dashboard load, and not refreshing when the grafana base dashboard refresh occure. (Only the dataset updated.)
In my case the overlay image updating dynamically (under a same name) but i can reload it only with user interruption (CTRL + F5).

It is possible to add somehow this feature?

Thank you so much!

@jreyesr
Copy link
Author

jreyesr commented May 17, 2021

@falconhome Hi! It's nice to know that someone else apart from me used the overlays :)

Just to check: as I understand it, your issue/enhancement is that, say, http://somepage.com/imgs/overlay host an image, which is updated periodically (but the URL stays the same), and you want to fetch the most recent version of the image every time the dashboard is manually or automatically refreshed (via the Refresh button or the timer), since it currently fetches the image one time, on page load, and the only way to refresh it is Ctrl+F5 or closing+reopening the tab.

@falconhome
Copy link

falconhome commented May 17, 2021

Yes, you understand correctly!
I tried to figure out, how can be add this function, but i failed on "yarn dev" + i did not find where the refresh should be implemented in a correct way.
Thanks a lot!
ps.: and the reason why i proposed the config settings too, because in many application they use static images and refreshing only the datas. In this case the periodically reload of the image is not required + it put extra load to the renderer...

@jreyesr
Copy link
Author

jreyesr commented May 17, 2021

OK. I think that it is possible to implement the described feature.

This PR was closed some months ago, and I don't think that it should handle more commits. Could you open an issue describing your feature request? That would get the official project maintainers involved (I am not a maintainer, by the way), and check if such an enhancement would be welcome or on scope for the project (I expect it would be, but it's probably a good idea to check with the maintainers first). You can mention that I am willing to implement the feature, since I developed the original overlay code, so there would be very little extra work for the maintainers.

After the issue is opened, we can take any further discussion there and leave this old PR to rest.

@falconhome
Copy link

Hi @jreyesr
I opened a ticket/feature request as you mentioned! ;)
#97
We can continue the conversation under that thread.
Thanks a lot!

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