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

fix: support older versions of android #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SamuelLeclerc33
Copy link

Since ?. wasn't supported before chrome 80, I changed the code injected in the browser that doesn't include it. While testing and investigating this, I also had to remove de use of max and env from the css since both of those where included in later version of chrome.

For reference :
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining
https://developer.mozilla.org/en-US/docs/Web/CSS/env
https://developer.mozilla.org/en-US/docs/Web/CSS/max

Since ?. wasn't supported before chrome 80, I changed the code injected in the browser that doesn't include it.
While testing and investigating this, I also had to remove de use of max and env from the css since both of those where included in later version of chrome.
@frederikheld
Copy link

frederikheld commented Oct 26, 2024

This is a design decision. I would highly recommend to add some docs about this. The next developer who submits a PR might use the ? not knowing about the compatibility concerns because that's how you code today.

IMHO the way better approach would be to add a polyfill for ? or (they don't seem to exist for operators) use Babel to transpile it (see https://babeljs.io/docs/babel-plugin-transform-optional-chaining).

This would allow to keep writing modern code and also wouldn't cause more bugs down the line if people don't remember this design decision.

@frederikheld
Copy link

frederikheld commented Oct 26, 2024

Furthermore: is Chrome 80 equivalent to WebView 80? WebView 80 was introduced in Android 5.0, so it only affects Android < 5.0. Capacitor 6 supports Android 5.1+

So there is no overlap between devices on Chrome > 80 and devices supported by Capacitor 6. Do those devices even have a notch?

If my assumption about WebView 80 ~ Chrome 80 is correct, I would advocate about merging this PR.

It just makes the code more complicated with no apparent use case. The plugin barely works as of now, we should not make it more complex for some edge case.

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.

2 participants