AllAboutLearningPress / Photo-Storage-and-Gallery

Share photo assets with all users in an easy to use gallery with a powerful backend.
3 stars 2 forks source link

Changes to `pig.js` #33

Closed dyner closed 3 years ago

andreyvolokitin commented 3 years ago

For adding/removing/reordering images it will likely require reinstantiating a whole thing (maybe with a spinner), so no dynamic things with this lib at its current state (maybe it can be relatively easily modified, but I'm in doubt about it, and the original author suggested this path: https://github.com/schlosser/pig.js/issues/28 )

For "selecting" it might be an easier modification

Also while at it, dynamic resizing (with a slider) can be also investigated (it resizes itself on window resize, so it's already a half-way there, perf still might not be fabulous, but that can be unimportant)

Not sure if the google photos "grouping" feature is required (probably not), where pics are grouped by date and also can be selected in bulk this way?: https://i.imgur.com/ZhEvivp.png . This feature is also absent from the lib

Can try to take a shot at all this, not very sure about result though

dyner commented 3 years ago

Not sure if the google photos "grouping" feature is required (probably not), where pics are grouped by date and also can be selected in bulk this way?: https://i.imgur.com/ZhEvivp.png . This feature is also absent from the lib

Can try to take a shot at all this, not very sure about result though

@andreyvolokitin I don't think grouping like that is going to be beneficial. We should allow Crtl + Shift select to select a range.

andreyvolokitin commented 3 years ago

We should allow Crtl + Shift select to select a range.

Didn't get this part, can you elaborate?

dyner commented 3 years ago

We should allow Crtl + Shift select to select a range.

Didn't get this part, can you elaborate?

For example, if you select item 1, and then hold Ctrl + Shift and click item 5, items 1-5 will all be selected. You can see how this works on your desktop or in Google Photos.

AshrafAkon commented 3 years ago

@andreyvolokitin While you work on the bug on pig.js. I will work on to make it dynamic. I think the author suggested that way because he assumed pig.js will be used as a library and wont be modified for that case.

But as we have much more images than the Authors's primary project and we intend to add more images We would need to make it dynamic. I have reviewed the code and this can be done. And I am working to implement it with the backend. This shouldn't interfere with your bug fix. But please let me know how you intend to do the upgrades/bug-fixes that you mentioned.

Vue.js has internal methods that does will help us make it more dynamic and I have an algorithm in mind for that. Hopefully, that will work.

andreyvolokitin commented 3 years ago

I'm in high doubt about vue bringing any reasonable benefit here, unless you intend to rewrite pig.js with a vue. The bug can be fixed externally, lib doesn't need a modification for that. The details of upgrades/modifications can be described after they're basically done, so not sure what you meant there. But basically extending/rewriting parts of the lib to allow:

If all this would be implemented properly, it will be somewhat similar to a google photos grid, so it might be quite hard. As I said, I can try to take a shot at this, but if you strictly against that idea, then feel free to do it

AshrafAkon commented 3 years ago

I understand your concern. But if not vue how do you propose to connect it with the backend? With potentially 10000 images at the beginning where we need to generate a pre-signed URL for accessing the images? Because the approach used by the Author assumes that you have all the image URLs on load. But in our case, we don't have it and we need to generate those URLs.

All of things you have said can be done in pure js. After all vue.js is just a js library. But vue.js handles the updating/modifying part of the dom element. So its much efficient to work with. As it is widely used for that purpose. Also its easier to maintain. If you write some part of it to modify the dom then I would need to somehow create a dual approach where two classes access the dom. Which would be a really bad design in terms of architecture. So i suggest that you should fix the bug that you noticed and let me handle the integration part of it.

andreyvolokitin commented 3 years ago

I obviously can't see a whole project picture, so if you say it — I agree. Okay, this case is probably the most strong case for vuejs in this whole project, so be it.

P.S.: the bug can be fixed in the source, but it is not required, because a single line after the lib initialisation can fix it. But probably the internal lib fix will make more sense in a light of a rewrite, so I'll investigate it

AshrafAkon commented 3 years ago

Initial edit for fetching new images on scroll added.