Tooltip Feature for ImageOverlays #1321
Conversation
Leaflet.distortable image feature
rebuild to resolve conflict
…re-listener Update listeners.html
|
Hi @jywarren, this PR is now ready for merging. Many thanks! |
.gitignore
Outdated
| @@ -1,6 +1,7 @@ | |||
| node_modules | |||
| coverage | |||
| todo.txt | |||
| observation.txt | |||
There was a problem hiding this comment.
Ah, is this something we need in the shared .gitignore or just for you? I'm ok either way i guess, I'm just thinking it's not 100% necessary to share.
There was a problem hiding this comment.
Oh! yeah... I'd would remove these fast, thanks for spotting them @jywarren.
examples/archive.css
Outdated
| position: absolute; | ||
| } | ||
|
|
||
| /* .tooltipText { |
There was a problem hiding this comment.
Should we remove this if you're not using it?
There was a problem hiding this comment.
Surely, I will remove it fast, thanks!
examples/archive.html
Outdated
| @@ -61,12 +62,14 @@ <h2 class="modal-title">Welcome to MapKnitter Lite</h2> | |||
| <i title="Open Sidebar" id="mapToggle" class="fa fa-bars fa-3x " style="position: absolute; right: 0; top:30px; margin: 1rem; z-index: 900; color: white; cursor: pointer;" aria-hidden="true"></i> | |||
|
|
|||
| <div id="map" style="width:100%; height:100%; position:absolute; top:0;"></div> | |||
| <!-- <div id="imageOverlaytooltip" class="distortableImageTooltip"><span id="tooltiptext" class="tooltiptext"></span></div> --> | |||
There was a problem hiding this comment.
Yes, please. I will, thanks!
examples/js/archive.js
Outdated
| @@ -127,6 +136,10 @@ function showImages(getUrl) { | |||
| axios.get(url) | |||
| .then((response) => { | |||
| if (response.data.files && response.data.files.length != 0) { | |||
| response.data.files.forEach(() => { | |||
| imageTooltipText = response.data.metadata.description; | |||
There was a problem hiding this comment.
Hmm, wait, if this is defined as a single variable at the scope of the whole page, don't we overwrite it with each image?
There was a problem hiding this comment.
I guess we need a way to save this description in a way that associates it with each image? Couldn't we assign a new property to response.data.files?
Hmm. Wait. Originally this was supposed to tell us about the individual image (from the description in #1317). Now i realize it's adding the description of the whole collection, isn't it? Maybe we can simplify things if we skip these four lines, and instead use the filename as the tooltip. Then it's already stored as part of each entry in imageThumbnails fullResImages below, right? That would be significantly simpler code!
There was a problem hiding this comment.
It appears files in archive.org/details/texas-barnraising/ have same description so the tooltip will show same description for each of the files in the said folder when placed on the tiled layer. However, if you fetch images with individual descriptions (such as in the gif illustration #4 above), then the correct tooltip (i.e., description of image) is displayed for each of the images.
Okay, so great idea, I will use the filename as the tooltip text. Users will only be required to save their images to internet archive using suggestive names . Many thanks!
| const imageURL = event.target.previousElementSibling.src; | ||
| image = L.distortableImageOverlay( | ||
| imageURL, | ||
| {tooltipText: imageTooltipText} |
There was a problem hiding this comment.
If we used the filename, we could just split event.target.previousElementSibling.src with split('/') and take any text after the final / - saving us a lot of complexity above, see?
There was a problem hiding this comment.
Nice.... I'll give this a shot. Many thanks!
src/DistortableImageOverlay.js
Outdated
| @@ -146,7 +153,7 @@ L.DistortableImageOverlay = L.ImageOverlay.extend({ | |||
| }, | |||
|
|
|||
| isSelected() { | |||
| return this._selected; | |||
| return this._selected; // this._selected | |||
There was a problem hiding this comment.
Just a comment to be removed here, thanks!
And a few other console.log() statements throughout as well!
There was a problem hiding this comment.
Yes, please... I will remove them all. Thanks!
examples/js/archive.js
Outdated
| @@ -1,4 +1,6 @@ | |||
| let map; | |||
| let image; | |||
| let imageTooltipText; | |||
There was a problem hiding this comment.
This can be removed if we follow some of the steps I outlined lower in this file to simplify things...
There was a problem hiding this comment.
Sure... I will, thanks!
src/DistortableCollection.js
Outdated
| @@ -2,6 +2,7 @@ const arr = []; | |||
| L.DistortableCollection = L.FeatureGroup.extend({ | |||
| options: { | |||
| editable: true, | |||
| tooltipControl: document.createElement('button'), | |||
There was a problem hiding this comment.
Hi @segun-codes -- I appreciate this toggling system here, and I know it's really challenging to dig into the guts of this library, and you've done a really really great job writing well-organized, easy to read code here. So congratulations.
However, I am just thinking... weighing complexity vs. benefit in balance, I ultimately think it's not worth it to have a permanent button-management system within the core library code. I have to think of what else people might do with tooltips, if they'd work the same way, if this code would need maintenance in the distant future, and so on... and ultimately you really solved most of the problem of the tooltips potentially interfering with things by making tooltips default off.
I'd like to ask you to keep the tooltip /creation/ code as it is in src/DistortableImageOverlay.js, but to remove the UI code from this file, and the button toggling the tooltips. I think it presents the user with another button which has a function not related to actual mapmaking, and I worry it's just not worth the added complexity, both for code, and for the user having to learn what the button is for.
I hope you understand and agree -- and once more, you've done a stellar job at architecting this UI and managing it, and integrating code into a complex library. I just think the better judgement call is to keep our changes to the core library simple and concise.
Thank you, Segun!!
There was a problem hiding this comment.
Oh! @jywarren, your line of thought is quite inspiring, I like it. Many thanks! So I guess it's okay to move the button-management system to archive.js, right?
And yeah, I do appreciate your commendations, worth a lot for me.
There was a problem hiding this comment.
For now, can you save the button management code in an issue, and we can wait and see if any issues ever come up with the tooltips obscuring or blocking things, and we can make a decision about including the button code later?
That will also keep this PR a bit narrower in scope and still achieve our main goal, but give us flexibility to respond later if we find the tooltips need more work. How does that sound? Thank you!
There was a problem hiding this comment.
Sounds okay, since it's for experimental purposes for the time being.
There was a problem hiding this comment.
For now, can you save the button management code in an issue, and we can wait and see if any issues ever come up with the tooltips obscuring or blocking things, and we can make a decision about including the button code later?
That will also keep this PR a bit narrower in scope and still achieve our main goal, but give us flexibility to respond later if we find the tooltips need more work. How does that sound? Thank you!
@jywarren, I have reverted to the earlier implementation (with some improvements) consistent with your advice. Kindly review and perhaps, consider for merging too. Please, view the gif file below. And as advised, here's the new issue #1329.
Illustration 5
|
🎉 🎉 🎉 🎉 |
Fixes #1317
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
grunt testThe GIFs below show the work done so far. However, the tooltip feature is also enabled in files
listeners.html,select.html,local.html, andexport.html.index.htmlarchive.htmlIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!