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

replace event.keyCode with event.code, tweak documentation #851

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

Conversation

mengwong
Copy link
Contributor

@mengwong mengwong commented Mar 5, 2023

resolves #849

@mengwong
Copy link
Contributor Author

mengwong commented Mar 5, 2023

sigh, fighting with linter, will push again in a moment.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@janishutz
Copy link
Contributor

janishutz commented Mar 5, 2023

Thank you for doing this. It's important to keep things up to date with the newest spec of ES6. When was this feature introduced? I'm tho kinda worried about impress stopping to work in older browsers. If this is a breaking change, please add a note to it in the README.md, the GettingStarted.md guide and the DOCUMENTATION.md files so people are informed about it.

@mengwong
Copy link
Contributor Author

mengwong commented Mar 5, 2023

Thank you for doing this. It's important to keep things up to date with the newest spec of ES6. When was this feature introduced? I'm tho kinda worried about impress stopping to work in older browsers. If this is a breaking change, please add a note to it in the README.md, the GettingStarted.md guide and the DOCUMENTATION.md files so people are informed about it.

It looks like all browsers added support in the 2015–2017 era, the last holdout was Edge in 2020. Would you consider it a breaking change?

Copy link
Contributor

@janishutz janishutz left a comment

Choose a reason for hiding this comment

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

There are errors in the test I don't quite understand, please look into it. I assume you might have missed a key-event and that's why it does not complete the tests, but I am not sure. https://app.circleci.com/pipelines/github/impress/impress.js/210/workflows/011fdf79-48e8-4a0f-a2ba-913b55905d9a/jobs/441

@mengwong
Copy link
Contributor Author

mengwong commented Mar 5, 2023

I am trying really hard to reproduce this, I don't understand it either. I am sure it will be facepalmingly obvious once it is solved.

@janishutz
Copy link
Contributor

What I expected to be a problem with this solution: https://caniuse.com/?search=keyboardevent.code

This will break impress.js in older browsers like IE and mobile browsers (aka. Safari for iOS, Firefox for Android, etc.)

Compare it to: https://caniuse.com/?search=keyboardevent.keyCode, which is supported by almost every major browser out there.

We should consider implementing a way to notify the user about incompatible plugins & automatically disable the plugins relying on these features, if the browser does not support it to retain support for old browsers with the most recent versions of impress.js.

Although this is great to offer, I personally am in favour of dropping support for the old browsers barely anyone uses any more nowadays. People that need to use those browsers should use a different CDN-Link or V2.0.0 instead of a new (future) release or upstream.

@janishutz
Copy link
Contributor

I am trying really hard to reproduce this, I don't understand it either. I am sure it will be facepalmingly obvious once it is solved.

That might happen. That's just life when developing stuff lol. Randomly troubleshooting problems that do not exist is still worse though imo

@mengwong
Copy link
Contributor Author

mengwong commented Mar 5, 2023

Re support for old browsers I feel like this project in particular should be able to afford upgrades since in most cases presentations tend to be ephemeral and are delivered by authors who are in control of both the browser and the npm deps. Presentations that need to be archival-quality could pin to a release version but the risk of bitrot tends to run in the opposite direction, so staying current is better.

@janishutz
Copy link
Contributor

Exactly

@janishutz
Copy link
Contributor

@henrikingo ready to merge, should work now

@henrikingo
Copy link
Member

Oh, apologies, I've been ignoring this PR as the checks are still shown as failed. If tests really are passing for you, then could you please trigger a new CircleCI build so that it is shown on this PR as well? You can do that easily by pushing a new commit with just a whitespace change somewhere.

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.

event.keyCode is deprecated, should upgrade to event.code
3 participants