Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Text overlay for GIF #1533

Merged
merged 2 commits into from
Jan 18, 2020
Merged

Text overlay for GIF #1533

merged 2 commits into from
Jan 18, 2020

Conversation

ataata107
Copy link

@ataata107 ataata107 commented Jan 18, 2020

Fixes #1462 (<=== Replace 0000 with the Issue Number)
text-overlay (5)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/is-reviewers for help, in a comment below
  • Insert-step functionality is working correct as expected.

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!

@ataata107
Copy link
Author

@jywarren @harshkhandeparkar @keshav234156 Text-overlay is also now working for gifs

@codecov
Copy link

codecov bot commented Jan 18, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@cffff05). Click here to learn what that means.
The diff coverage is 66.07%.

Impacted file tree graph

@@          Coverage Diff           @@
##             main   #1533   +/-   ##
======================================
  Coverage        ?   66.7%           
======================================
  Files           ?     129           
  Lines           ?    2664           
  Branches        ?     428           
======================================
  Hits            ?    1777           
  Misses          ?     887           
  Partials        ?       0
Impacted Files Coverage Δ
examples/lib/insertPreview.js 13.51% <ø> (ø)
src/Modules.js 100% <ø> (ø)
examples/lib/defaultHtmlStepUi.js 10.98% <0%> (ø)
src/util/getImageDimensions.js 20% <0%> (ø)
src/modules/DrawRectangle/DrawRectangle.js 100% <100%> (ø)
src/modules/Histogram/Module.js 100% <100%> (ø)
src/modules/Crop/Module.js 88.88% <100%> (ø)
src/modules/ConstrainedCrop/index.js 100% <100%> (ø)
src/modules/Contrast/Module.js 100% <100%> (ø)
src/modules/Dynamic/Module.js 78.94% <100%> (ø)
... and 5 more

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! This is great. How hard do you think it'd be to write a test for this?

@root00198
Copy link
Member

I will create a test in #1530 once this pr gets merged

@ataata107
Copy link
Author

ataata107 commented Jan 18, 2020

@jywarren ya I guess it will be possible
@root00198 sure maybe we can collaborate?

@root00198
Copy link
Member

Yes we can.You should add a text-overlay.js in test/core/gifs and when the PR's will get merged it will work automatically. Follow the format which is used in my PR for the text-overlay.js.

@ataata107
Copy link
Author

Ok cool
Let’s wait for this to get merged

@jywarren
Copy link
Member

Ok! Awesome! Thanks!!!

@harshkhandeparkar
Copy link
Member

Is it required? Also, the way it is implemented isn't the most optimal solution. It would be better to pass in a function that calculates the datauri on demand if required. Calculating the datauri for every frame and for every module even though the majority of the modules do not even use this feature.

@jywarren
Copy link
Member

Maybe this needs it's own issue? Is this in #1547 now? thanks!

frames[f] = options.extraManipulation(framePix, setRenderState, generateOutput, datauri) || framePix; // extraManipulation is used to manipulate each pixel individually.

@rishabhshuklax
Copy link
Member

@jywarren Yes

@ataata107
Copy link
Author

It is required for modules which needs url, (in case of gifs, getting url through canvas method wasnt working well) . @harshkhandeparkar yes it can be refactored so as to not being processed for other modules.

@harshkhandeparkar
Copy link
Member

@ataata107 could you please open an issue for the refactor?

@rishabhshuklax
Copy link
Member

@harshkhandeparkar it has already been done! #1551

@harshkhandeparkar
Copy link
Member

Nope. I was talking about this

Also, the way it is implemented isn't the most optimal solution. It would be better to pass in a function that calculates the datauri on demand if required. Calculating the datauri for every frame and for every module even though the majority of the modules do not even use this feature.

@ataata107 ataata107 mentioned this pull request Jan 24, 2020
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text-Overlay Does Not Work With GIFs
5 participants