Conversation
Reviewer's Guide by SourceryThis pull request introduces an OnEvent callback feature to the Player component. This allows consumers to subscribe to various player events (such as 'ready', 'play', 'pause', 'ended', 'enterfullscreen', 'exitfullscreen', and 'languagechange') via a .NET callback. The implementation involves setting up JavaScript event listeners for the underlying Plyr player, which then invoke a .NET method using JSInterop. This .NET method subsequently triggers the user-provided OnEvent delegate. The JavaScript event handling is conditionally enabled based on a data-bb-event attribute on the player element. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Review the logic of the EventString property. (link)
Overall Comments:
- The logic controlling event propagation via the
data-bb-eventattribute seems unnecessarily complex and potentially inverted; consider always invoking the C# handler from JavaScript.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 1 other issue
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| player.destroy(); | ||
| player = null; |
There was a problem hiding this comment.
issue (bug_risk): Avoid reassigning a destructured variable in dispose.
Since 'player' is a destructured const, nulling it doesn’t affect p. To clear the reference, assign null to p.player.
| [Parameter] | ||
| public Func<string, Task>? OnEvent { get; set; } | ||
|
|
||
| private string? EventString => OnEvent == null ? "true" : null; |
There was a problem hiding this comment.
question (bug_risk): Review the logic of the EventString property.
The mapping is inverted: ‘true’ is set when OnEvent is null and cleared when a callback exists. Swap these so JS events fire only when a callback is provided.
| }); | ||
| } | ||
|
|
||
| const handlerEvents = p => { |
There was a problem hiding this comment.
issue (complexity): Consider inlining the event registration logic into a single loop to simplify the code.
Consider inlining the event registration logic into a single loop instead of creating two helper functions. This reduces one layer of abstraction without altering functionality. For example, instead of having separate functions:
const handlerEventName = (name, p) => {
const { el, invoke, method, player } = p;
player.on(name, () => {
const fire = el.getAttribute('data-bb-event') === 'true';
if (fire) {
invoke.invokeMethodAsync(method, name);
}
});
}
const handlerEvents = p => {
['ready', 'play', 'pause', 'ended', 'enterfullscreen', 'exitfullscreen', 'languagechange']
.forEach(name => {
handlerEventName(name, p);
});
}You can inline the logic like this:
const handlerEvents = (p) => {
const { el, invoke, method, player } = p;
['ready', 'play', 'pause', 'ended', 'enterfullscreen', 'exitfullscreen', 'languagechange']
.forEach(name => {
player.on(name, () => {
if (el.getAttribute('data-bb-event') === 'true') {
invoke.invokeMethodAsync(method, name);
}
});
});
}This change maintains the same behavior while simplifying the code structure.
| delete options.source; | ||
| if (hls) { | ||
| if (source.length > 0) { | ||
| const src = source[0].src; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const src = source[0].src; | |
| const {src} = source[0]; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
1 participant
Link issues
fixes #421
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add an
OnEventcallback to the Player component to allow subscribing to player events.New Features:
OnEventparameter (Func<string, Task>) to receive notifications for player events like 'play', 'pause', 'ended', 'enterfullscreen', 'exitfullscreen', and 'languagechange'.Enhancements:
disposeandsetPosterfunctions.