load image in sidebar as thumbnail #1319
Conversation
|
@jywarren What are your thoughts on this implementation of the image render in the sidebar? |
|
Hello @jywarren here is a draft for the server side pagination #1300 For a link with images over 100
For a link with images below 100
So upon your review of this PR - #1319, this would be integrated. Thank you! |
|
Wow, this is quite impressive, i really like the filtering of thumbnails into I have two requests -- first, we are working with some much bigger blocks of code here; can you use a couple comments to explain what each portion does? For example, line 117-141 is so dense -- what does each section do? How can we make this code more readable and maintainable? The second is -- are there any ways we can reduce the number of lines here by creating re-usable functions? Can we get clever with how the HTML is generated to make it easier to reuse across the 3 different renderers, since that seems to be the largest source of new lines of code? Thanks a lot for this complex and carefully written PR -- I'm hoping we can refine it a little bit before we move forward! |
|
@jywarren Thanks a lot for your review, as per your requests, of course, I will add the comments and break them into chunks of reusable code. Thanks again |
|
@jywarren I've added the comments and made the adjustments to the code for readability and maintainability, I think you'd like it |
There was a problem hiding this comment.
Hi @7malikk -- I've made some pretty big suggestions here, and I hope they make sense. Code style is not the kind of thing where there's a "right" answer, and everybody does things a little differently. But what I tried to explain is how I like to use comments to explain mysterious things, but to keep comments as short as possible by using descriptive variable and function names. And the structure of the code itself (not too many nested functions, similar named functions for related functionality) can help to make things readable.
This is looking really good. Let me know what you think about my review and suggestions. Thanks a lot for sticking with this one, we're almost there!
examples/js/archive.js
Outdated
| function renderImages(files, url, count) { | ||
| // thumbs variable is to extract the thumbnail files from the fetched data | ||
| const thumbs = files.filter(file => file.source === 'derivative'); | ||
| // images variable is to extract the actual image files from the fetched data |
There was a problem hiding this comment.
Here, I think we can skip the comment if we use a very clear variable name - i'm a big fan of longer variable names that are very descriptive -- that way they're legible wherever in the codebase they're used. So what if we skipped the comments and named these variables thumbnail_images and original_images? (I would use camelCase but the lower case l and the capital I are hard to read...
There was a problem hiding this comment.
Sounds good, thanks @jywarren, yea camelCase there is usually hard to read
examples/js/archive.js
Outdated
| // images variable is to extract the actual image files from the fetched data | ||
| const images = files.filter(file => file.format === 'PNG' || file.format === 'JPEG'); | ||
|
|
||
| // this if check is to check the amount of image files and thumbnail files is below 100, afterwhich if true it renders the images in the sidebar |
There was a problem hiding this comment.
I'm just thinking -- I feel like I want to have two functions named renderImages and renderThumbnails, and to use comments to show that the first is just plain images, and the second searches for thumbnails if they exist. The current renderImages is mostly just logic switching between those two, isn't it? And the thumbs filter could go into renderThumbnails because it's more specific to that task.
The logic (if loop) for which of those two functions we use could be more simply written:
// if over 100 images, show only thumbnails
if (count > 100) renderThumbnails(images, url);
else renderImages(images, url);Then it's quite readable too - and this could go down on line 152, eliminating this intermediary function. Then we have 2 functions which are similarly named and act similarly, although one uses thumbnails instead. What do you think?
This would also keep all the filtering code together, which seems good too.
There was a problem hiding this comment.
I think I like the idea, it does make the code more readable.
I will work on the implementation, hopefully, it'll be a homerun from there.
Thank you @jywarren
@jywarren Thanks a lot for the review and the suggestions, they make a lot of sense as it helps demystify the code. |
Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>
Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>
Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>
|
@jywarren made a refactor, and ended with a little chunky code but it seems readable and understandable, let me know what you think. While refactoring I realized in other for the logic to fail gracefully into full-resolution images, I needed to filter initially, before the separate function calls. |
Fixes #1295
grunt testUI changes when the images fetched are above 100