teach-la-website
teach-la-website copied to clipboard
uclaacm
Image sizing ci
@nicolasjeong is this PR ready to review? I'm not sure if we've hit all the requested items in the ticket yet!
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)
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.
@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.
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!
@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 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.
It seems that one of the check still fails? I'm pretty sure it's because of the non-fixable images that have errors.
Just touching back to this, @reginawang99 / @vivianha534 are y'all wrapping up this ticket? Or do you want me to finish it?
@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)
@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.
Just curious, what ended up happening with this PR? @vivianha534 @reginawang3495