resize before loading previews #1180
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Now the webpage doesnt fail and gives the preview even for very large image but it does take some time |
|
@aashna27 is it based on what we discussed? |
Codecov Report
@@ Coverage Diff @@
## main #1180 +/- ##
==========================================
- Coverage 55.78% 55.45% -0.34%
==========================================
Files 114 114
Lines 2352 2366 +14
Branches 363 364 +1
==========================================
Hits 1312 1312
- Misses 1040 1054 +14
|
|
@aashna27 thanks for your appreciation. So, cheers for this. |
Hehe Thanks @Divy123 for motivating and also please guide me for your pr, I will look for sources!! |
|
Yeah sure. I will leave some comments today and clearly state what I am
currently working on
Thanks a lot once again.
…On Sat 20 Jul, 2019, 11:47 PM aashna27, ***@***.***> wrote:
@aashna27 <https://github.com/aashna27> thanks for your appreciation.
Though I believe we are a team and it feels great to see this working. I
think you have done a fantastic job and removed a big restriction to the
usage of IS.
So, cheers for this.
Hehe Thanks @Divy123 <https://github.com/Divy123> for motivating and also
please guide me for your pr, I will look for sources!!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1180?email_source=notifications&email_token=AHOHJLYCPAANJZ474EKZ7Q3QANJC3A5CNFSM4IFPN2MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2NTNTA#issuecomment-513488588>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHOHJL225P5MZW3OWDB6BHLQANJC3ANCNFSM4IFPN2MA>
.
|
|
This is awesome teamwork!
…On Sat, Jul 20, 2019, 2:21 PM Slytherin ***@***.***> wrote:
Yeah sure. I will leave some comments today and clearly state what I am
currently working on
Thanks a lot once again.
On Sat 20 Jul, 2019, 11:47 PM aashna27, ***@***.***> wrote:
> @aashna27 <https://github.com/aashna27> thanks for your appreciation.
> Though I believe we are a team and it feels great to see this working. I
> think you have done a fantastic job and removed a big restriction to the
> usage of IS.
>
> So, cheers for this.
>
> Hehe Thanks @Divy123 <https://github.com/Divy123> for motivating and
also
> please guide me for your pr, I will look for sources!!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <
#1180?email_source=notifications&email_token=AHOHJLYCPAANJZ474EKZ7Q3QANJC3A5CNFSM4IFPN2MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2NTNTA#issuecomment-513488588
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AHOHJL225P5MZW3OWDB6BHLQANJC3ANCNFSM4IFPN2MA
>
> .
>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1180?email_source=notifications&email_token=AAAF6J7J4R4DYGMUCPBJTF3QANJPZA5CNFSM4IFPN2MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2NTPGI#issuecomment-513488793>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6J3ON5CHW6PU253EWITQANJPZANCNFSM4IFPN2MA>
.
|
|
Is this good to merge? Very exciting!!
…On Sat, Jul 20, 2019, 2:25 PM Jeffrey Warren ***@***.***> wrote:
This is awesome teamwork!
On Sat, Jul 20, 2019, 2:21 PM Slytherin ***@***.***> wrote:
> Yeah sure. I will leave some comments today and clearly state what I am
> currently working on
>
> Thanks a lot once again.
>
> On Sat 20 Jul, 2019, 11:47 PM aashna27, ***@***.***> wrote:
>
> > @aashna27 <https://github.com/aashna27> thanks for your appreciation.
> > Though I believe we are a team and it feels great to see this working. I
> > think you have done a fantastic job and removed a big restriction to the
> > usage of IS.
> >
> > So, cheers for this.
> >
> > Hehe Thanks @Divy123 <https://github.com/Divy123> for motivating and
> also
> > please guide me for your pr, I will look for sources!!
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <
> #1180?email_source=notifications&email_token=AHOHJLYCPAANJZ474EKZ7Q3QANJC3A5CNFSM4IFPN2MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2NTNTA#issuecomment-513488588
> >,
> > or mute the thread
> > <
> https://github.com/notifications/unsubscribe-auth/AHOHJL225P5MZW3OWDB6BHLQANJC3ANCNFSM4IFPN2MA
> >
> > .
> >
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#1180?email_source=notifications&email_token=AAAF6J7J4R4DYGMUCPBJTF3QANJPZA5CNFSM4IFPN2MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2NTPGI#issuecomment-513488793>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAAF6J3ON5CHW6PU253EWITQANJPZANCNFSM4IFPN2MA>
> .
>
|
|
Yeah I believe that it is good to merge! |
jywarren
reviewed
Jul 20, 2019
examples/lib/insertPreview.js
Outdated
| var sequencer = ImageSequencer(); | ||
|
|
||
| sequencer.loadImage(src, function(){ | ||
| this.addSteps('resize', {['resize']: '30%'}); |
There was a problem hiding this comment.
Should we limit to a fixed size? Like 100x100? Because really large images at 30% are still quite large!
There was a problem hiding this comment.
but idt we have that option in resize.
|
Can we calculate it using image width?
…On Sat, Jul 20, 2019, 2:42 PM aashna27 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/lib/insertPreview.js
<#1180 (comment)>
:
> @@ -44,9 +47,17 @@ function updatePreviews(src, selector) {
}
};
- Object.keys(previewSequencerSteps).forEach(function (step, index) {
- generatePreview(step, Object.values(previewSequencerSteps)[index], src, selector);
+ var sequencer = ImageSequencer();
+
+ sequencer.loadImage(src, function(){
+ this.addSteps('resize', {['resize']: '30%'});
but idt we have that option in resize.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1180?email_source=notifications&email_token=AAAF6J5447J5CK7MXKKL5I3QANL6ZA5CNFSM4IFPN2MKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB7CA66I#discussion_r305589341>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6J4O76FOO3QDH52FZXDQANL6ZANCNFSM4IFPN2MA>
.
|
|
Hey @jywarren I thought about this and I believe that for this I need to change the input values for resize! And take it as 2 dimensions width and height, instead of the currently accepting percentage value. Do you think there is any other way? Because in the insertPreview file we are just passing the image src. Okay I did find something will implement it! Thanks. |
|
Hmm. I guess I thought based on image dimensions we could calculate the
percentage. But the resize module should perhaps accept absolute dimensions
anyways. Maybe we can set a units parameter? As either pixels or
percentage?
But what do you think of merging this first then doing that as a follow-up?
…On Sat, Jul 20, 2019, 11:49 PM aashna27 ***@***.***> wrote:
Hey @jywarren <https://github.com/jywarren> I thought about this and I
believe that for this I need to change the input values for resize! And
take it as 2 dimensions width and height, instead of the currently
accepting percentage value. Do you think there is any other way?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1180?email_source=notifications&email_token=AAAF6J4PIXGUDIWWGHFVKMLQAPMCBA5CNFSM4IFPN2MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2N2LIY#issuecomment-513516963>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6JZGVOSU44IJYRBCH3LQAPMCBANCNFSM4IFPN2MA>
.
|
|
Yeah actually that was my point but the i thought that for that we will
need our inputs to be dynamic, ie if user selects percentage then just ask
for perc value and if absolute then ask height and width. And this would
require great changes to the currently accepting input js we have!
But i am thinking maybe give the src to dimensions a try. And if that
doesn't work out we can merge this and I will do the absolute dimensions in
a follow-up.
Relying on this
https://stackoverflow.com/questions/17774928/js-get-image-width-and-height-from-the-base64-code
for dimensions from img src.
On Sun, Jul 21, 2019, 9:23 AM Jeffrey Warren <notifications@github.com>
wrote:
… Hmm. I guess I thought based on image dimensions we could calculate the
percentage. But the resize module should perhaps accept absolute dimensions
anyways. Maybe we can set a units parameter? As either pixels or
percentage?
But what do you think of merging this first then doing that as a follow-up?
On Sat, Jul 20, 2019, 11:49 PM aashna27 ***@***.***> wrote:
> Hey @jywarren <https://github.com/jywarren> I thought about this and I
> believe that for this I need to change the input values for resize! And
> take it as 2 dimensions width and height, instead of the currently
> accepting percentage value. Do you think there is any other way?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <
#1180?email_source=notifications&email_token=AAAF6J4PIXGUDIWWGHFVKMLQAPMCBA5CNFSM4IFPN2MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2N2LIY#issuecomment-513516963
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AAAF6JZGVOSU44IJYRBCH3LQAPMCBANCNFSM4IFPN2MA
>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1180?email_source=notifications&email_token=AGKQ7SABGWJXZ2JFUCEIM2DQAPMSLA5CNFSM4IFPN2MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2N2N4I#issuecomment-513517297>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGKQ7SFIUXGBC7S42ZFPBALQAPMSLANCNFSM4IFPN2MA>
.
|
|
@jywarren I have made the changes and tested it on all types of image sizes.
|
|
Awesome!!!! |
jywarren
approved these changes
Jul 22, 2019
|
Super. We can merge to gh-pages soon! |
|
Yeah many new features |
jywarren
pushed a commit
that referenced
this pull request
Dec 16, 2019
* resize before loading previews * min resize percentage calculated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #922 & #1122
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test@publiclab/is-reviewersfor help, in a comment belowIf 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!