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

feat: Support for Web #48

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

Conversation

zhuhaichao518
Copy link

I am developing an app requiring web support for gamepads. I would like to contribute to this project. Please help review this pull request. Thanks!

Copy link
Member

@erickzanardo erickzanardo left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I will be super cool to have support on web too, I have left some comments, let me know of anything

packages/gamepads/example/pubspec.yaml Outdated Show resolved Hide resolved
packages/gamepads_web/lib/gamepad_detector.dart Outdated Show resolved Hide resolved
flutter_web_plugins:
sdk: flutter
gamepads_platform_interface: ^0.1.2+1
js: ^0.6.3
Copy link
Member

Choose a reason for hiding this comment

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

I believe that the dart:js package is deprecated: https://dart.dev/interop/js-interop

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for reminding! I have updated to use js_interop and package:web instead of package:js and dart:html.

packages/gamepads/example/pubspec.yaml Outdated Show resolved Hide resolved
packages/gamepads_web/CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 57 to 64
final gamepads = navigator.getGamepads();
return List.generate(gamepads.length, (i) {
final gamepad = gamepads[i];
if (gamepad != null) {
return gamepad;
}
return null;
}).where((gamepad) => gamepad != null).toList();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final gamepads = navigator.getGamepads();
return List.generate(gamepads.length, (i) {
final gamepad = gamepads[i];
if (gamepad != null) {
return gamepad;
}
return null;
}).where((gamepad) => gamepad != null).toList();
return navigator.getGamepads().toDart.whereNotNull();

Shouldn't this be enough?

Copy link
Author

Choose a reason for hiding this comment

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

I did't notice that I can use gamepad in package:web. Updated the implementation.

return controllers;
}

List<dynamic> getGamepadList() {
Copy link
Member

Choose a reason for hiding this comment

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

Can't the list be typed here?

Copy link
Author

@zhuhaichao518 zhuhaichao518 Oct 24, 2024

Choose a reason for hiding this comment

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

I did't get it well. Does the latest implementation look fine?

packages/gamepads_web/lib/gamepad_detector.dart Outdated Show resolved Hide resolved
packages/gamepads_web/lib/gamepads_web.dart Outdated Show resolved Hide resolved
packages/gamepads_web/lib/gamepads_web.dart Outdated Show resolved Hide resolved
@zhuhaichao518
Copy link
Author

@spydon @erickzanardo Any more suggestions?

import 'package:gamepads_platform_interface/gamepads_platform_interface.dart';
import 'package:web/web.dart';

List<GamepadController> getGamepads(GamepadsPlatformInterface plugin) {
Copy link
Member

Choose a reason for hiding this comment

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

This file should live in src right? And not be exposed directly to the user, since it's not part of the interface.

}
}

/// Constructs a GamepadsWeb
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Constructs a GamepadsWeb

The dart docs should talk about what it does, docs like "constructor constructs" doesn't give anything, so I think you can just remove this.

@spydon
Copy link
Member

spydon commented Dec 10, 2024

Also this:

Because every version of gamepads_web from path depends on web >=0.1.0-beta which requires SDK version >=3.1.0-49.0.dev <4.0.0, gamepads_web from path is forbidden.

So the SDK has to be bumped to 3.4 everywhere, and then you can use web ^1.1.0 (And flutter has to be bumped accordingly).

@markvideon
Copy link
Collaborator

Popping in to mention that there is a Github issue in this project associated with web support. I made a proof of concept for a web implementation earlier this year and I wouldn't consider web to be a stable platform for the purposes of this package, see comments on the issue.

#32

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.

4 participants