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

Feature/59 create with html element instead of id only #697

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

bilal-elchami
Copy link
Contributor

This PR is made to support sending the HTML Element of the reader directly to the library instead of the element-id string to avoid problems when using shadowRoot.
An example was submitted (examples/html5-shadow-dom)

The issue details can be found here #59

Please note that sending the element-id as a string is still compatible in this version.

@all-contributors please add @bilal-elchami for this feature.

Copy link

@InDIOS InDIOS left a comment

Choose a reason for hiding this comment

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

This PR looks good, the approach is very useful in cases where we need to pass an element on demand or need to choose the element from a component template such as Web Component or Angular Component templates. 👌

Copy link

@InDIOS InDIOS left a comment

Choose a reason for hiding this comment

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

Nice work, still compatible with old code. It looks very well.

@mebjas
Copy link
Owner

mebjas commented Feb 24, 2023

Thanks for the PR.

Can you re-sync with the latest changes. Happy to do the review and merge.

@bilal-elchami
Copy link
Contributor Author

Hello @mebjas
I justed synced the latest changes into my branch.
Thanks

README.md Outdated Show resolved Hide resolved
examples/html5-shadow-dom/README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
minified/html5-qrcode.min.js Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
if (this.lastMatchFound === decodedText) {
return;
}
if (qrCodeSuccessCallback) {
Copy link
Owner

Choose a reason for hiding this comment

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

Are these changes driven by some tool or manual?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just formatted the file using the default VsCode TypeScript formatter.
image

* @param config Extra configurations to tune the code scanner.
* @param verbose - If true, all logs would be printed to console.
*/
public constructor(
Copy link
Owner

Choose a reason for hiding this comment

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

For my information, what does this constructor overload with no implementation do in typescript?

Copy link
Contributor Author

@bilal-elchami bilal-elchami Mar 1, 2023

Choose a reason for hiding this comment

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

The idea was to have multiple constructors with a different signature each.
So in order to implement this constructor overloads, we couldn't have two constructors with bodies. You have to create the constructors signatures and then a global constructor that will handle the whole logic.
An example can be found here: https://timmousk.com/blog/typescript-multiple-constructors/

The purpose of adding multiple constructors is having the following IntelliSense suggestions:
image
image

I can remove this overloading and use one constructor instead if you prefer with the suggested signature:
public constructor(elementOrElementId: HTMLElement | string, config: Html5QrcodeScannerConfig | undefined, verbose: boolean | undefined)

Waiting for your response

@bilal-elchami
Copy link
Contributor Author

Hello @mebjas
I just resolved your comments.
Btw building the project on the latest version is not compiling properly the dist\html5-qrcode.min.js
Any idea if it's related to the newly pushed changes?

@mebjas
Copy link
Owner

mebjas commented Mar 4, 2023

Btw building the project on the latest version is not compiling properly the dist\html5-qrcode.min.js

What issue are you seeing?

@bilal-elchami
Copy link
Contributor Author

Btw building the project on the latest version is not compiling properly the dist\html5-qrcode.min.js

What issue are you seeing?

After running the npm run-script build command on the latest master version, the dist/html5-qrcode.min.js content is as follow:

if (window) { if (!Html5QrcodeScanner) { var Html5QrcodeScanner = window.__Html5QrcodeLibrary__.Html5QrcodeScanner; } if (!Html5Qrcode) { var Html5Qrcode = window.__Html5QrcodeLibrary__.Html5Qrcode; } if (!Html5QrcodeSupportedFormats) { var Html5QrcodeSupportedFormats = window.__Html5QrcodeLibrary__.Html5QrcodeSupportedFormats } if (!Html5QrcodeScannerState) { var Html5QrcodeScannerState = window.__Html5QrcodeLibrary__.Html5QrcodeScannerState; } if (!Html5QrcodeScanType) { var Html5QrcodeScanType = window.__Html5QrcodeLibrary__.Html5QrcodeScanType; }}

Which isn't the complete compiled library.

@bilal-elchami
Copy link
Contributor Author

Hello @mebjas ! Any updates about this PR? And please note that the compilation problem is not related to this PR.

@webermax
Copy link

webermax commented Apr 23, 2023

...we are eagerly waiting for this to work, too.

(The following example isn't working yet (?): https://github.com/mebjas/html5-qrcode/tree/master/examples/lit)

@cemkalyoncu
Copy link

cemkalyoncu commented Jun 22, 2023

@bilal-elchami

I fixed that problem by installing webpack first.

npm install --save-dev webpack

I also had to change lib version from es7 to es2018 in tsconfig.json

@webermax
Copy link

@bilal-elchami

I fixed that problem by installing webpack first.

npm install --save-dev webpack

I also had to change lib version from es7 to es2018 in tsconfig.json

Which problem do you mean (the custom element example does not involve any build process at all)?

(Still getting "Uncaught (in promise) HTML Element with id=[object HTMLDivElement] not found"...)

@cemkalyoncu
Copy link

Bilal commented about not being able to build and got mostly empty .min.js file. This is to fix that problem. If you want that system, you should use Bilal's repository at his feature branch and compile it yourself to get the min.js, the one in repo is from the original one. I managed to get it working.

@bilal-elchami
Copy link
Contributor Author

@bilal-elchami

I fixed that problem by installing webpack first.

npm install --save-dev webpack

I also had to change lib version from es7 to es2018 in tsconfig.json

Hello @cemkalyoncu
I was trying to sync my branch with the latest version of master and tried what you suggested in order to have a complete build of the dist/html5-qrcode.min.js, but I didn't manage to make it work.

Can you tell me the steps you took to make it build properly?

Thanks

@bilal-elchami
Copy link
Contributor Author

Hello @mebjas

I just merged the latest version of master into my branch and the PR is now up to date.

@cemkalyoncu
Copy link

After last merge, the error about TS version error has disappeared. Here are the steps I have taken to build:

npm install --save-dev webpack
npm run-script build

The minified file is built into dist folder.

@TheodoreGC
Copy link

TheodoreGC commented Sep 18, 2023

Is this PR ready to be merged?

@bilal-elchami , @mebjas

@bilal-elchami
Copy link
Contributor Author

bilal-elchami commented Sep 18, 2023

Is this PR ready to be merged?

@bilal-elchami , @mebjas

Hello @TheodoreGC , I updated the branch on the 29th of June and we're waiting for @mebjas to review it.
Once reviewed, I'll sync it with the latest version of master.

Meanwhile, if you would like to use or test it, my project uses a custom branch on my forked repo.
package.json dependency: https://gitpkg.now.sh/bilal-elchami/html5-qrcode/dist?mobee (via https://gitpkg.vercel.app/)

@ROBERT-MCDOWELL
Copy link

ROBERT-MCDOWELL commented Sep 18, 2023

wouldn't be more logic and easy that @mebjas delegate his repo to trusty guys like you since there are here more than 3.7k users/developers following it?

@mebjas
Copy link
Owner

mebjas commented Sep 18, 2023

Any suggestions?

@ROBERT-MCDOWELL
Copy link

for example give the rights to the most active PR guys here?

@TheodoreGC
Copy link

Thank you @bilal-elchami for the update and the efforts you have put into this.

Giving rights to the most active PR contributors sounds more practical for speeding up reviews and the merging process.

@mebjas, do you have any idea if/when this could be implemented?

@vmandrespascual
Copy link

Thank you @bilal-elchami !
I find this request very interesting and the code seems to work correctly for me.

@mebjas, it really raises the inclusion at the end?
It makes a lot of sense, how can the community help you to get this to the main branch?

Best regards

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.

9 participants