Skip to content
This repository has been archived by the owner on Aug 27, 2022. It is now read-only.

Improve JavaScript #88

Closed
wants to merge 3 commits into from
Closed

Improve JavaScript #88

wants to merge 3 commits into from

Conversation

andreasremdt
Copy link
Collaborator

@andreasremdt andreasremdt commented Jul 31, 2020

Hey Andi! I started to work a little bit in the JavaScript side of things. Don't be surprised by the amount of changes, it's mostly moving files around. I did the following:

  • Installed gulp-babel and @babel/env. These are used for JavaScript transpilation, meaning that you can use ES6 (like const, let, etc.) while having support for older browsers, such as Safari 9.
  • Changed the Gulp pipeline to also transpile the JavaScript.
  • Moved the raw, unprocessed Sass and JavaScript files into a new folder src. When Gulp has ran, it will put the processed (and ready-to-use) files into resources/(js|css), like they where before. Meaning that from now on, you should edit the Sass in and JavaScript in the src directory and let Gulp put them into resources.
  • Updated simple-translator to version 2.0.1. I released this version yesterday and it now supports more browsers, is already transpiled and can be loaded from a CDN. The API has also slightly changed.
  • I removed the file i18n-sub.js and instead set the config filesLocation to /photobooth/resources/lang, meaning that it will use the absolute path. On my system, the root folder is called "photobooth", but I am not sure how this project is used on other people machine's. If we need to go back to relative paths and having this sub-file, please let me know.

Things are still not perfect, I would see this as the first iteration.
If you have any questions or if something is not working, let me know, I'll try to fix it then :)

Installs Babel and @babel/env to transpile JavaScript into ES5 code
Adapts the Gulp pipeline to process JavaScript
Moves source files into the `src` folder
Removes async/await and imports
Install version 2.0.1 of simple translator via CDN
Improve usage
@andreasremdt andreasremdt changed the title Add build pipeline Improve JavaScript Jul 31, 2020
@andi34
Copy link
Owner

andi34 commented Jul 31, 2020

Hey and thanks for your work!
Can we use npm for simple-translator? Photobooth often is used without internet access. 8539532
Does yarn eslint still work?

Photobooth is installed in most cases as the only webpage inside /var/www/html , via install script it could also be a subpage inside /var/www/html/photobooth, so we should restore the previous setup in that direction.
About the need of "i18n-sub.js": I had the problem the language files haven't been loaded on some pages. I would have to test again on your new version of simple-translator.

Again thanks a lot for your work!

I'll test your changes ASAP :)

@andi34
Copy link
Owner

andi34 commented Jul 31, 2020

8539532

yarn eslint doesn't work, I'll take a look at it tomorrow.
Also thinking about adding resources/js/ to .gitignore

@andi34
Copy link
Owner

andi34 commented Aug 1, 2020

Edit:
For eslint: faff7f0

@andreasremdt
Copy link
Collaborator Author

Sorry, I forgot to change the path for the ESLint script to the src directory...

Yeah, simple-translator is available via npm, but I already saw that you added it to the PR 👍

I'd recommend adding the /resources/js folder to the .gitignore, but this means that the files won't be available in the repository anymore. Depending on how the users install this project this might be an issue? I guess they would have to run npm install && npm run build first?

ESLint currently fails because the Translator class is available globally, but it doesn't know this. There are two ways of solving this:

  1. Adding it to the .eslintrc like so: https://eslint.org/docs/user-guide/configuring#specifying-globals
  2. Adding the comment /*eslint no-undef: "off"*/ in the affected file.

I'd recommend the first method, since it's a legit global variable. I'll fix that as soon as possible, and I will also look at the i18n-sub.js that we talked about.

@andi34
Copy link
Owner

andi34 commented Aug 1, 2020

Hey and thanks!
faff7f0
Eslint now works.

I also ignored /resources/js.
If we run "yarn pack:build" the resources/js folder will be added to the release package.
On a installation via git people had to run "yarn build" anyway to generate the css files as we also haven't tracked them here. So there should be no issue.

All changes to your PR
https://github.com/andi34/photobooth/commits/add-build-pipeline
(Maybe merge via "rebase & merge" as PR on top of yours? So we can merge it later into dev without multiple "merge branch xxx into xxx" commits)

Will have to test further later about the translator working. Only looked at the general code parts until now.

@andi34
Copy link
Owner

andi34 commented Aug 1, 2020

Another thing I've seen you added to your simple-translator:

There's a way to let GitHub test PRs? Not sure if it can be added to atomatically run eslint and tell if there's an error before a PR get merged.

Edit: done
https://github.com/andi34/photobooth/blob/dev/.github/workflows/lint.yml
https://github.com/andi34/photobooth/actions/runs/191451706

@andi34 andi34 self-assigned this Aug 1, 2020
@andi34 andi34 added the enhancement New feature or request label Aug 1, 2020
@andi34
Copy link
Owner

andi34 commented Aug 1, 2020

Only quick tested with the PR on top of your branch, everything seems to work.
Will have to test a little bit more in the evening :)

@andi34
Copy link
Owner

andi34 commented Aug 2, 2020

On ipad2 (Safari 9) simple translator does not work.
#47 (comment)

Waiting for more feedback.

@andi34
Copy link
Owner

andi34 commented Aug 2, 2020

I found that Safari 9 throws the following error from translator.min.js

ReferenceError: Can't find variable: fetch 

After a bit of digging, it looks to me that the root cause may be iOS 9 / Safari 9 did not yet implement the fetch() API (Fetch API compatibility matrix)

There is a polyfill available that from a very first glance could be a solution to backwards compatibility - fetch

The best way I found to debug iPad2 in the Photobooth is actually via USB connected to the Pi it self, and using the instructions right here: remotedebug-ios-webkit-adapter

@andreasremdt
Copy link
Collaborator Author

Closed by #90

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants