Image sizing ci #331

nicolasjeong · 2021-04-21T04:21:53Z

No description provided.

mattxwang · 2021-05-02T03:22:31Z

@nicolasjeong is this PR ready to review? I'm not sure if we've hit all the requested items in the ticket yet!

nicolasjeong · 2021-05-02T05:07:26Z

Oh my bad, I thought I had to submit a PR after resolving some of the comments from checkImageSize.js before moving on to fixImageSize.js. (Unless I missed something in checkImageSize.js)

mattxwang · 2021-05-02T06:50:45Z

Oh my bad, I thought I had to submit a PR after resolving some of the comments from checkImageSize.js before moving on to fixImageSize.js. (Unless I missed something in checkImageSize.js)

Sure, in that case I can give a light review, but we shouldn't merge this until we also make a working fixImageSize.js action and resolve the issues with the CI.

mattxwang

Left some comments! Also cc: @vivianha534

package.json

test/checkImageSize.js

vivianha534

I'm going to defer to @mattxwang because I'm not very familiar with CI, but I left a couple of comments. Adding whether or not a file is fixable, is more of a nit.

vivianha534 · 2021-05-10T05:54:54Z

test/checkImageSize.js

+for (const file of finalFilesArray){ //Check the image size of all image files
+    const imageStatus = imageSizeChecker(file);
+    if (imageStatus.error){
+        console.error("Error: " + file + " -- " + imageStatus.message+ "\n");


It would be nice to also let the person know if their error is fixable, so that way they know if they need to manually fix the issue or if they can run fixImageSize to fix it.

vivianha534 · 2021-05-10T06:02:45Z

test/checkImageSize.js

+    else if(isImageWidthLarge(imageSize)){
+        return errorType.LARGE_IMAGE_ERROR;
+    }
+    else if (!isAValidExtension(input)){


This if statement would also fail if the file had a non .jpg, .jpeg, or .png extension (ie. it would fail if they had a .svg file) However, the error message for EXTENSION_ERROR only mentions file name not having a lower case extension name.

The getAllFiles() function(line 86) deals with this since it returns an array of the file names that have .jpg, jpeg, or .png. Therefore, the array won't contain any file that has a non .jpg, .jpeg, or .png extension. We will run imageSizeChecker to files that only have the .jpg, .jpeg, or .png extension.

vivianha534 · 2021-05-13T20:11:45Z

@reginawang99 @ellieyhcheng I'm not sure why the export/require statements are causing everything in checkImageSize.js to run instead of just the getAllFiles function.

reginawang3495 · 2021-05-13T20:21:31Z

Kk I can take a look tonight

mattxwang · 2021-05-13T20:39:57Z

Oh I can jump in here if that makes things easier, basically the entire file is being executed on interpretation (afaik how importing works with ESM/node, but I could be wrong)

I would suggest instead to separate concerns:

one file should just be a shared library that only exports functions
two individual files should just be the main routines (i.e. calling the modules from that file)

Ideally, any file you import shouldn't have any deterministic or side-effect behaviour!

vivianha534 · 2021-05-13T20:47:30Z

@mattxwang Oh ok, so if I'm understanding this correctly, if we had functions that we wanted to use in both checkImageSize.js and fixImageSize.js then we should define the function inside of another file like library.js or something and export the functions in there, and require library.js in checkImageSize.js and fixImageSize.js in order to use the exported functions?

mattxwang · 2021-05-13T20:49:21Z

@mattxwang Oh ok, so if I'm understanding this correctly, if we had functions that we wanted to use in both checkImageSize.js and fixImageSize.js then we should define the function inside of another file like library.js or something and export the functions in there, and require library.js in checkImageSize.js and fixImageSize.js in order to use the exported functions?

Yup! I would also say that this is generally best practice! I would put all relevant library files in your lib.js file or similar.

Also, since we're not supporting Node 10, ESM imports/exports are probably the way to go.

…ant functions to checkImageSize.js and fixImageSize.js, and finished fixImageSize.js

mattxwang

Left some comments! Most of them are nitpicky, though the behaviour around failing fixes is pretty important (I think try-catch and log is probably fine in this instance).

Also, a minor nit is that we're using let very frequently when const would suffice, which helps with "defensive coding"!

I think if you address these comments, we're prob good to go. After you make the changes (tag me again, but I think this is good), you should:

merge master into image-sizing-ci
then, run the fixer
ideally, commit that night

test/checkImageSize.js

test/fixImageSize.js

test/library.js

Co-authored-by: Matt Wang <matt@matthewwang.me>

mattxwang

Feel free to pull the trigger on merging + autofixing!

nicolasjeong · 2021-05-22T06:15:22Z

It seems that one of the check still fails? I'm pretty sure it's because of the non-fixable images that have errors.

mattxwang · 2021-05-27T20:31:33Z

Just touching back to this, @reginawang99 / @vivianha534 are y'all wrapping up this ticket? Or do you want me to finish it?

reginawang3495 · 2021-05-27T20:44:04Z

@vivianha534 i think we can just fix that one fixable image, and just push? and we manually fix/ask people with pictures that are too small for updated pics?

(we can probably manually fix the non-square aspect ratios)

vivianha534 · 2021-05-27T23:04:36Z

@mattxwang @reginawang99 Oh I was planning on manually fixing all of the images before having Nico merge it. I've already been working on fixing the images, and the last couple should be done by tonight/tomorrow latest.

mattxwang · 2021-11-16T05:36:45Z

Just curious, what ended up happening with this PR? @vivianha534 @reginawang3495

nicolasjeong added 4 commits April 20, 2021 21:10

First draft

6fba60c

Added github action

a68d995

Added process.exit

532b529

Made edits to chechImageSize

fb3923f

nicolasjeong requested review from vivianha534 and mattxwang May 2, 2021 02:17

mattxwang requested changes May 2, 2021

View reviewed changes

Added enums to checkImageSize.js and resolved comments

8bb727e

vivianha534 suggested changes May 10, 2021

View reviewed changes

mattxwang marked this pull request as draft May 10, 2021 06:19

Trying to export checkImageSize functions to fixImageSize

b745c58

nicolasjeong added 5 commits May 18, 2021 10:01

Trying to add a library.js file and use import/export to import modules

d121fe4

Made a library.js file with all the functions, imported all the relev…

4fa7afd

…ant functions to checkImageSize.js and fixImageSize.js, and finished fixImageSize.js

Final edits to fixImageSize.js

92bd0a2

Final edits to library.js

e20689d

Final edits

749383b

mattxwang requested changes May 19, 2021

View reviewed changes

nicolasjeong and others added 4 commits May 18, 2021 20:21

Update test/fixImageSize.js

cea9618

Co-authored-by: Matt Wang <matt@matthewwang.me>

Resolved comments

5388979

Made a few edits

84a6235

Final Edits

25d5a1e

nicolasjeong marked this pull request as ready for review May 20, 2021 01:34

mattxwang reviewed May 20, 2021

View reviewed changes

nicolasjeong added 2 commits May 21, 2021 22:57

Commiting checkImageSize and fixImageSize ticket

37a0ce9

Merged with master branch and fixed images

833d2c9

Vivian Ha and others added 5 commits November 16, 2021 17:07

resized and/or cropped photos that were failing the test

677eb4d

updated mli photo

501adf2

updated mli to be png instead of jpg

46cf793

Merge branch 'master' into image-sizing-ci

c9e207a

Merge branch 'image-sizing-ci'

3b028f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image sizing ci #331

Image sizing ci #331

nicolasjeong commented Apr 21, 2021

mattxwang commented May 2, 2021 •

edited

Loading

nicolasjeong commented May 2, 2021

mattxwang commented May 2, 2021

mattxwang left a comment

vivianha534 left a comment

vivianha534 May 10, 2021

vivianha534 May 10, 2021

nicolasjeong May 10, 2021

vivianha534 commented May 13, 2021 •

edited

Loading

reginawang3495 commented May 13, 2021 •

edited

Loading

mattxwang commented May 13, 2021

vivianha534 commented May 13, 2021

mattxwang commented May 13, 2021

mattxwang left a comment

mattxwang left a comment

nicolasjeong commented May 22, 2021

mattxwang commented May 27, 2021

reginawang3495 commented May 27, 2021 •

edited

Loading

vivianha534 commented May 27, 2021

mattxwang commented Nov 16, 2021

Image sizing ci #331

Are you sure you want to change the base?

Image sizing ci #331

Conversation

nicolasjeong commented Apr 21, 2021

mattxwang commented May 2, 2021 • edited Loading

nicolasjeong commented May 2, 2021

mattxwang commented May 2, 2021

mattxwang left a comment

Choose a reason for hiding this comment

vivianha534 left a comment

Choose a reason for hiding this comment

vivianha534 May 10, 2021

Choose a reason for hiding this comment

vivianha534 May 10, 2021

Choose a reason for hiding this comment

nicolasjeong May 10, 2021

Choose a reason for hiding this comment

vivianha534 commented May 13, 2021 • edited Loading

reginawang3495 commented May 13, 2021 • edited Loading

mattxwang commented May 13, 2021

vivianha534 commented May 13, 2021

mattxwang commented May 13, 2021

mattxwang left a comment

Choose a reason for hiding this comment

mattxwang left a comment

Choose a reason for hiding this comment

nicolasjeong commented May 22, 2021

mattxwang commented May 27, 2021

reginawang3495 commented May 27, 2021 • edited Loading

vivianha534 commented May 27, 2021

mattxwang commented Nov 16, 2021

mattxwang commented May 2, 2021 •

edited

Loading

vivianha534 commented May 13, 2021 •

edited

Loading

reginawang3495 commented May 13, 2021 •

edited

Loading

reginawang3495 commented May 27, 2021 •

edited

Loading