Connected pre-loaded images to v2#436
Connected pre-loaded images to v2#436stephaniequintana wants to merge 3 commits intopubliclab:mainfrom
Conversation
|
|
|
@jywarren, @TildaDares, @cesswairimu Clarifications/Discussion requested:
I hope this finds you all well :) |
|
Hi, no issue, it's confusing! Merging will incorporate only whats shown in the changed files here, and the merge action always prioritizes the active PR's changes. You should just before merging be able to see in the diff view the green + lines that will be the final state after the merge is conducted. Once a merge is completed, the diff view and changed files in all other PRs will be modified to show the new post-merge changes taking into account the action that just happened. Does that make sense? It's OK to include changes in both - it's very likely they'll then be dropped from the diff of the other, as that change will already have occurred, OR it may say there's a conflict and it can't be merged automatically. |
Should we try this idea of detecting what version we're running? I'll make a code suggestion so the flag will exist! |
|
I started the flag implementation in #438 - i will try to generate the dist files too -- but if that makes sense we can merge it and you can begin using |
|
@jywarren, Please ensure that the function was placed in the correct location. I've also include it in the dist/infragram.js file in preparation of dist/infragram2.js becoming obsolete. The code does not break the original version and thus I did not utilize the version flag (implemented in #438). You can view this in action at https://stephaniequintana.github.io/infragram/index2.html |
jywarren
left a comment
jywarren
left a comment
There was a problem hiding this comment.
Hi @stephaniequintana it took me a moment to catch up to you understanding the code here but it looks great. I made some suggestions for simplicity, legibility and such. If you take up the suggestion of eliminating thumbnail images, we can further simplify the HTML too, removing the red-filter classes I had added and a few of the ones you added as well.
Let me know what you think!
src/ui/interface.js
Outdated
| return true; | ||
| }); | ||
|
|
||
| $("#sample-image__select").click(function(e){ |
There was a problem hiding this comment.
This looks good; it'll only run if there is an HTML DOM element with an id of sample-image__select, so this won't break v1. However, 2 thoughts -
- should we mark this as a v2 feature with a comment, so people know, and if we ever clean up all v1 code we can tell it apart
- should we otherwise explain this briefly in a comment, since it sounds a little mysterious?
There was a problem hiding this comment.
What's cool is that this means ANY image could be used as a sample image, it just has to have the id sample-image__select. I suggest that we change that to a class, since strictly, ids should be used only once. Then someday someone may want to add, for example, a bunch of different examples in a kind of menu, they could use this. So the class could be maybe... .sample-image?
There was a problem hiding this comment.
But perhaps that means we should then apply the class to just the individual images, rather than the whole container. Let me make a suggestion!
src/ui/interface.js
Outdated
|
|
||
| $("#sample-image__select").click(function(e){ | ||
| e.stopPropagation(); | ||
| const img = (e.target.classList.contains('rfi')) ? document.getElementById('rfi') : document.getElementById('bfi'); |
There was a problem hiding this comment.
Here's another super great line of code - i love that the idea is that you can add the "bfi" or "rfi" classes to switch between. Maybe we should explain this too in a comment!
There was a problem hiding this comment.
Aha - i see now -- we want to click on the thumbnail, but use the big hidden image! I wonder - if we use relatively low res sample images, like, 1024x768 or less, could we simplify things by just skipping thumbnails? JPGs don't weigh that much, you know? Then the logic here can be much simpler - we just display the big images with style="width:200px;" or something like that!
There was a problem hiding this comment.
So on this line, if we stop using separate thumbnails, we can maybe just skip using red-filter or blue-filter classes completely. Since we'd just select whatever image, and think about what kind of filter later?
index2.html
Outdated
| <p class="card-text" style="font-size:.625rem;padding-top:.5rem;"><small>Red filtered trees</small></p> | ||
| <div id="sample-image__select" class="d-flex justify-content-around"> | ||
| <div id="red-filtered-img" class="rfi d-flex flex-column justify-content-center align-items-baseline"> | ||
| <img class="rfi" src="assets/red-filtered-trees-thumbnail.jpeg" alt="Red filtered image - select to explore the tool." style="width:5rem;height:auto;border:.5rem solild var(--image-container-bg);"> |
There was a problem hiding this comment.
| <img class="rfi" src="assets/red-filtered-trees-thumbnail.jpeg" alt="Red filtered image - select to explore the tool." style="width:5rem;height:auto;border:.5rem solild var(--image-container-bg);"> | |
| <img class="sample-image red-filter" src="assets/red-filtered-trees-thumbnail.jpeg" alt="Red filtered image - select to explore the tool." style="width:5rem;height:auto;border:.5rem solild var(--image-container-bg);"> |
I explain this idea below - and as to naming, I really like longer descriptive names because it often means we don't have to use as many comments!
index2.html
Outdated
| </div> | ||
| <div class="d-flex flex-column justify-content-center align-items-center"> | ||
| <div id="blue-filtered-img" class="d-flex flex-column justify-content-center align-items-center"> | ||
| <img src="assets/blue-filtered-plant-thumbnail.jpeg" alt="Blue filtered image - select to explore the tool." style="width:5rem;height:auto;"> |
There was a problem hiding this comment.
| <img src="assets/blue-filtered-plant-thumbnail.jpeg" alt="Blue filtered image - select to explore the tool." style="width:5rem;height:auto;"> | |
| <img class="sample-image blue-filter" src="assets/blue-filtered-plant-thumbnail.jpeg" alt="Blue filtered image - select to explore the tool." style="width:5rem;height:auto;"> |
| const file = new File([blob], img.src, blob) | ||
| const fileInput = document.querySelector('input[type="file"]'); | ||
| const dataTransfer = new DataTransfer(); | ||
| dataTransfer.items.add(file); |
There was a problem hiding this comment.
Wow i really like this way of doing it. It literally selects the image, same as someone using it! That will mean that we don't have to support (or think about) 2 different means of adding images. Very cool.
|
Great suggestions! Thank you 🙏 I originally put the eventListener on the entire div in case a user clicks the text (instead of the image), but this came with having to add many classes to many elements. This way is MUCH cleaner and the function itself is more clear. for your quick reference, it is now: $(".sample-image").click(function(e){
e.stopPropagation();
const img = (e.target);
fetch(img.src)
.then(res => res.blob())
.then(blob => {
const file = new File([blob], img.src, blob)
const fileInput = document.querySelector('input[type="file"]');
const dataTransfer = new DataTransfer();
dataTransfer.items.add(file);
fileInput.files = dataTransfer.files;
$(options.fileSelector).trigger("change");
})
});I also deleted the thumbnails and replaced the images with smaller versions (width: 1024). I am working on a better design for the welcome-container as a whole and will include this in a separate UI PR. |
|
Sounds super!!!
…On Thu, Aug 4, 2022, 5:46 PM Stephanie Quintana ***@***.***> wrote:
@jywarren <https://github.com/jywarren>,
Great suggestions! Thank you 🙏
I originally put the eventListener on the entire div in case a user clicks
the text (instead of the image), but this came with having to add many
classes to many elements. This way is MUCH cleaner and the function itself
is more clear. for your quick reference, it is now:
$(".sample-image").click(function(e){
e.stopPropagation();
const img = (e.target);
fetch(img.src)
.then(res => res.blob())
.then(blob => {
const file = new File([blob], img.src, blob)
const fileInput = document.querySelector('input[type="file"]');
const dataTransfer = new DataTransfer();
dataTransfer.items.add(file);
fileInput.files = dataTransfer.files;
$(options.fileSelector).trigger("change");
})
});
I also deleted the thumbnails and replaced the images with smaller
versions (width: 1024).
I am working on a better design for the welcome-container as a whole and
will include this in a separate UI PR.
—
Reply to this email directly, view it on GitHub
<#436 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6J2YWVOA6CLSXRUKZLTVXQ24JANCNFSM54IQ4XXQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
2 participants
Allows the user to select a pre-loaded image to explore the tool with.
Fixes the "Connect Sample Images" from Planning issue #415
Caveats:
<input>element (see question in comments)@publiclab/reviewersfor help, in a comment below