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.

Sign up for GitHub

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

Jump to bottom

add snapshot processing options #56

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

add snapshot processing options #56

wants to merge 1 commit into from

Conversation

rawpixel-vincent
Copy link
Contributor

@rawpixel-vincent rawpixel-vincent commented Oct 6, 2022 •
edited
Loading

  • add --take-snapshot cli options list
  • apply jpeg compression
  • normalize screenshot size based on the screen dpi
  • apply max size boundaries
  • add ratio from original instead of the dpi in the screenshot filename
  • replace pngjs with sharp and support png or jpeg for the diff
  • docs
  • tests

would fix #48

@rawpixel-vincent rawpixel-vincent force-pushed the feat/snapshot-processing branch from d7b34dd to 94f4181 Compare October 6, 2022 14:45
@rawpixel-vincent rawpixel-vincent force-pushed the feat/snapshot-processing branch from 94f4181 to 4e24549 Compare October 6, 2022 15:20
@rawpixel-vincent
Copy link
Contributor Author

@pateketrueke you can check the options I added when you have time and let me know if it looks ok to you

pateketrueke
pateketrueke reviewed Oct 7, 2022
View reviewed changes
utils/parseSnapshotOptions.js
...parsed.reduce((acc, cur) => {
if (cur.quality && !isNaN(cur.quality)) {
cur.quality = parseInt(cur.quality, 10);
acc = { ...acc, ...cur };
Copy link
Collaborator

Choose a reason for hiding this comment

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

even if this works, you don't need to reassign the accumulator, only extend it, e.g.

+acc.quality = parseInt(cur.quality, 10);
-cur.quality = parseInt(cur.quality, 10);
-acc = { ...acc, ...cur };

the same happen with all options, and the code will be less 🐝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense 👌🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pateketrueke pateketrueke pateketrueke left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rawpixel-vincent @pateketrueke