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

Add sequenceId slot and fix effect promises #201

Open
wants to merge 2 commits into
base: gh-pages
Choose a base branch
from

Conversation

nondebug
Copy link
Collaborator

@nondebug nondebug commented Apr 27, 2024

Closes #200

The following tasks have been completed:

  • Modified Web platform tests (link to pull request)

Implementation commitment:


Preview | Diff

@nondebug nondebug requested a review from marcoscaceres May 9, 2024 20:30
@marcoscaceres
Copy link
Member

@gterzian, do you want to check this also?

@marcoscaceres
Copy link
Member

Will need to check if the original issue is observable in any browser or if there was code already preventing this. Might be good if we can come up with a test to confirm (seems it might be possible to do so).

@marcoscaceres
Copy link
Member

marcoscaceres commented May 20, 2024

@nondebug I'm still not sure what happens if the identifiers don't match... feels like there should be some kind of assert or something somewhere to assuring nothing is getting out of sync.

Copy link

@gterzian gterzian left a comment

Choose a reason for hiding this comment

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

LGTM, with a few nits to consider.

index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Adding @gterzian's suggestions would be great. Can review again after those updates.

@nondebug
Copy link
Collaborator Author

@gterzian Thanks for the suggestions!

@marcoscaceres Please take another look

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

approving, but we should add a test so I can check WebKit.

@marcoscaceres
Copy link
Member

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.

GamepadHapticActuator promises issues
3 participants