feat(DriverJs): add OverlayClickBehavior support#707
Merged
Conversation
Reviewer's GuideThis PR adds full support for overlay click behavior by extending the C# configuration API, wiring up a JSInvokable callback, updating the Blazor-to-JS interop layer, and replacing the embedded driver library with a new module that handles the configured behavior. Sequence diagram for overlay click behavior callback from JS to .NETsequenceDiagram
participant JS as "JavaScript (driver.js.mjs)"
participant Blazor as "Blazor Component (DriverJs.razor.js)"
participant DotNet as "C# DriverJs Component"
participant Config as "DriverJsConfig"
JS->>Blazor: overlayClickBehavior triggers
Blazor->>DotNet: invokeMethodAsync("OnOverlayClicked", index)
DotNet->>Config: OnOverlayClickedAsync(this, Config, index)
Note over DotNet,Config: Executes configured C# callback
Updated class diagram for DriverJs and DriverJsConfig overlay click supportclassDiagram
class DriverJs {
+Start(int? index = 0)
+OnDestroyed()
+OnOverlayClicked(int index)
}
class DriverJsConfig {
+OnDestroyedAsync : Func<Task>
+OverlayClickBehavior : string
+OnOverlayClickedAsync : Func<DriverJs, DriverJsConfig, int, Task>
}
DriverJs --> DriverJsConfig : uses
DriverJsConfig <|-- DriverJsConfig : OverlayClickCallbackMethod (private)
DriverJs o-- "1" DriverJsConfig : Config
DriverJsConfig ..> DriverJs : OnOverlayClickedAsync callback
File-Level Changes
Assessment against linked issues
Possibly 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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.DriverJs/Components/DriverJs.razor.cs:95-101` </location>
<code_context>
+ /// </summary>
+ /// <returns></returns>
+ [JSInvokable]
+ public async Task OnOverlayClicked(int index)
+ {
+ if (Config is { OnOverlayClickedAsync: not null })
+ {
+ await Config.OnOverlayClickedAsync(this, Config, index);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling exceptions in OnOverlayClicked to avoid unhandled errors.
Because this method is called from JavaScript, unhandled exceptions may cause unpredictable behavior. Use a try-catch around the await call to handle errors and improve reliability.
```suggestion
public async Task OnOverlayClicked(int index)
{
if (Config is { OnOverlayClickedAsync: not null })
{
try
{
await Config.OnOverlayClickedAsync(this, Config, index);
}
catch (Exception ex)
{
// TODO: Replace with your logging mechanism if available
Console.Error.WriteLine($"Error in OnOverlayClicked: {ex}");
}
}
}
```
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.DriverJs/Components/DriverJsConfig.cs:150` </location>
<code_context>
+ /// 获得/设置 点击遮罩行为 close nextStep function 默认 close
+ /// </summary>
+ [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
+ public string? OverlayClickBehavior { get; set; }
+
+ [JsonInclude]
</code_context>
<issue_to_address>
**suggestion:** OverlayClickBehavior is a string; consider using an enum for clarity and type safety.
Strings are prone to errors and complicate future changes; enums improve safety and code clarity.
Suggested implementation:
```csharp
/// <summary>
/// 获得/设置 点击遮罩行为，默认 Close。可选值: Close, NextStep, Function
/// </summary>
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public OverlayClickBehaviorType? OverlayClickBehavior { get; set; }
/// <summary>
/// 点击遮罩行为枚举
/// </summary>
public enum OverlayClickBehaviorType
{
Close,
NextStep,
Function
}
```
If this property is serialized/deserialized elsewhere, you may need to update those usages to handle the enum type instead of string. If you use this property in other files, update their references to use the enum values (e.g., `OverlayClickBehaviorType.Close` instead of `"close"`).
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.DriverJs/Components/DriverJs.razor.js:37-39` </location>
<code_context>
d.invoke.invokeMethodAsync("OnDestroyed");
};
}
+ if (overlayClickBehavior === "function") {
+ options.overlayClickBehavior = async (el, step, { state }) => {
+ await d.invoke.invokeMethodAsync("OnOverlayClicked", state.activeIndex)
+ };
+ }
</code_context>
<issue_to_address>
**suggestion:** The overlayClickBehavior check for "function" is string-based; consider aligning with a more robust type or value check.
Using a constant or enum for 'function' will help prevent typos and make future updates safer.
Suggested implementation:
```javascript
const OVERLAY_CLICK_BEHAVIOR_FUNCTION = "function";
d.config = config;
const { index } = config;
const { hookDestroyStarted, hookDestroyed, overlayClickBehavior } = options;
```
```javascript
if (overlayClickBehavior === OVERLAY_CLICK_BEHAVIOR_FUNCTION) {
options.overlayClickBehavior = async (el, step, { state }) => {
await d.invoke.invokeMethodAsync("OnOverlayClicked", state.activeIndex)
};
}
```
</issue_to_address>
### Comment 4
<location> `src/components/BootstrapBlazor.DriverJs/Components/DriverJs.razor.cs:81-88` </location>
<code_context>
public async Task OnDestroyed()
{
- if (Config?.OnDestroyedAsync != null)
+ if (Config is { OnDestroyedAsync: not null })
{
await Config.OnDestroyedAsync();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling exceptions in OnDestroyed to avoid unhandled errors.
Because OnDestroyed is called from JavaScript, exceptions here may propagate unexpectedly. Use a try-catch around the await to handle errors and improve robustness.
```suggestion
[JSInvokable]
public async Task OnDestroyed()
{
if (Config is { OnDestroyedAsync: not null })
{
try
{
await Config.OnDestroyedAsync();
}
catch (Exception ex)
{
// TODO: Replace with your preferred logging mechanism
Console.Error.WriteLine($"Exception in OnDestroyed: {ex}");
}
}
}
```
</issue_to_address>
### Comment 5
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:1` </location>
<code_context>
+let z = {}, J;
+function F(e = {}) {
+ z = {
</code_context>
<issue_to_address>
**issue (complexity):** Consider splitting the large driver.js file into focused modules for config, state, animation, DOM utilities, overlay, popover, and events.
Here are a few quick wins to break this 1.7 K‐line file into focused, testable modules without changing any logic:
1. Extract “config” helpers
```js
// config.js
const DEFAULT = {
animate: true,
allowClose: true,
overlayClickBehavior: 'close',
/* … */
};
let cfg = { ...DEFAULT };
export function setConfig(opts = {}) {
cfg = { ...cfg, ...opts };
}
export function getConfig(key) {
return key == null ? cfg : cfg[key];
}
```
2. Extract “state” store
```js
// state.js
let store = {};
export function setState(key, value) { store[key] = value; }
export function getState(key) { return key == null ? store : store[key]; }
export function resetState() { store = {}; }
```
3. Extract animation easing
```js
// animation.js
// formerly function O(e,o,t,i){…}
export function easeOutQuad(time, from, change, duration) {
time /= duration/2;
if (time < 1) return change/2*time*time + from;
return -change/2*(--time*(time-2)-1) + from;
}
```
4. Extract DOM‐utils (scroll, visibility, focusable) into `dom-utils.js`.
5. Group all overlay/SVG functions in `overlay.js` (we, ge, fe, te, he, oe, me).
6. Group all popover logic in `popover.js` (Le, Q, ae, Ee, se, Z, G, Te).
7. Pull event‐binding into `events.js` (re, ke, _e, Pe, ne, M, Se).
Then in your main `driver.js` you import:
```js
import { setConfig, getConfig } from './config.js';
import * as State from './state.js';
import { easeOutQuad } from './animation.js';
import * as Dom from './dom-utils.js';
import * as Overlay from './overlay.js';
import * as Popover from './popover.js';
import * as Events from './events.js';
function driver(userOpts) {
setConfig(userOpts);
// …all calls to s() ➔ getConfig(), L() ➔ State.setState(), etc.
}
export { driver as default };
```
Splitting this way:
- separates concerns,
- makes each file ~100 lines max,
- makes unit tests trivial,
- preserves 100% of existing logic.
</issue_to_address>
### Comment 6
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:34` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 7
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:166` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 8
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:193` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 9
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:203` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 10
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:219` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 11
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:478` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 12
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:484-486` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 13
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:487-503` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 14
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:504-510` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 15
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:511-517` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 16
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:518-520` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 17
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:521-536` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 18
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:522` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 19
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:537-552` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 20
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:538` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 21
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:553-555` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 22
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:556-593` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 23
<location> `src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs:594-619` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+95
to
+101
| public async Task OnOverlayClicked(int index) | ||
| { | ||
| if (Config is { OnOverlayClickedAsync: not null }) | ||
| { | ||
| await Config.OnOverlayClickedAsync(this, Config, index); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Consider handling exceptions in OnOverlayClicked to avoid unhandled errors.
Because this method is called from JavaScript, unhandled exceptions may cause unpredictable behavior. Use a try-catch around the await call to handle errors and improve reliability.
Suggested change
| public async Task OnOverlayClicked(int index) | |
| { | |
| if (Config is { OnOverlayClickedAsync: not null }) | |
| { | |
| await Config.OnOverlayClickedAsync(this, Config, index); | |
| } | |
| } | |
| public async Task OnOverlayClicked(int index) | |
| { | |
| if (Config is { OnOverlayClickedAsync: not null }) | |
| { | |
| try | |
| { | |
| await Config.OnOverlayClickedAsync(this, Config, index); | |
| } | |
| catch (Exception ex) | |
| { | |
| // TODO: Replace with your logging mechanism if available | |
| Console.Error.WriteLine($"Error in OnOverlayClicked: {ex}"); | |
| } | |
| } | |
| } |
| /// 获得/设置 点击遮罩行为 close nextStep function 默认 close | ||
| /// </summary> | ||
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
| public string? OverlayClickBehavior { get; set; } |
There was a problem hiding this comment.
suggestion: OverlayClickBehavior is a string; consider using an enum for clarity and type safety.
Strings are prone to errors and complicate future changes; enums improve safety and code clarity.
Suggested implementation:
/// <summary>
/// 获得/设置 点击遮罩行为，默认 Close。可选值: Close, NextStep, Function
/// </summary>
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public OverlayClickBehaviorType? OverlayClickBehavior { get; set; }
/// <summary>
/// 点击遮罩行为枚举
/// </summary>
public enum OverlayClickBehaviorType
{
Close,
NextStep,
Function
}If this property is serialized/deserialized elsewhere, you may need to update those usages to handle the enum type instead of string. If you use this property in other files, update their references to use the enum values (e.g., OverlayClickBehaviorType.Close instead of "close").
Comment on lines
+37
to
+39
| if (overlayClickBehavior === "function") { | ||
| options.overlayClickBehavior = async (el, step, { state }) => { | ||
| await d.invoke.invokeMethodAsync("OnOverlayClicked", state.activeIndex) |
There was a problem hiding this comment.
suggestion: The overlayClickBehavior check for "function" is string-based; consider aligning with a more robust type or value check.
Using a constant or enum for 'function' will help prevent typos and make future updates safer.
Suggested implementation:
const OVERLAY_CLICK_BEHAVIOR_FUNCTION = "function";
d.config = config;
const { index } = config;
const { hookDestroyStarted, hookDestroyed, overlayClickBehavior } = options; if (overlayClickBehavior === OVERLAY_CLICK_BEHAVIOR_FUNCTION) {
options.overlayClickBehavior = async (el, step, { state }) => {
await d.invoke.invokeMethodAsync("OnOverlayClicked", state.activeIndex)
};
}
Comment on lines
81
to
88
| [JSInvokable] | ||
| public async Task OnDestroyed() | ||
| { | ||
| if (Config?.OnDestroyedAsync != null) | ||
| if (Config is { OnDestroyedAsync: not null }) | ||
| { | ||
| await Config.OnDestroyedAsync(); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Consider handling exceptions in OnDestroyed to avoid unhandled errors.
Because OnDestroyed is called from JavaScript, exceptions here may propagate unexpectedly. Use a try-catch around the await to handle errors and improve robustness.
Suggested change
| [JSInvokable] | |
| public async Task OnDestroyed() | |
| { | |
| if (Config?.OnDestroyedAsync != null) | |
| if (Config is { OnDestroyedAsync: not null }) | |
| { | |
| await Config.OnDestroyedAsync(); | |
| } | |
| } | |
| [JSInvokable] | |
| public async Task OnDestroyed() | |
| { | |
| if (Config is { OnDestroyedAsync: not null }) | |
| { | |
| try | |
| { | |
| await Config.OnDestroyedAsync(); | |
| } | |
| catch (Exception ex) | |
| { | |
| // TODO: Replace with your preferred logging mechanism | |
| Console.Error.WriteLine($"Exception in OnDestroyed: {ex}"); | |
| } | |
| } | |
| } |
| I[e] = o; | ||
| } | ||
| function L(e) { | ||
| var o; |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
Comment on lines
+537
to
+552
| function w() { | ||
| var m; | ||
| if (l("__transitionCallback")) | ||
| return; | ||
| const p = l("activeIndex"), c = l("__activeStep"), u = l("__activeElement"); | ||
| if (typeof p == "undefined" || typeof c == "undefined") | ||
| return; | ||
| const h = ((m = c.popover) == null ? void 0 : m.onNextClick) || s("onNextClick"); | ||
| if (h) | ||
| return h(u, c, { | ||
| config: s(), | ||
| state: l(), | ||
| driver: _() | ||
| }); | ||
| i(); | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.| d(); | ||
| } | ||
| function w() { | ||
| var m; |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
Comment on lines
+553
to
+555
| function r() { | ||
| l("isInitialized") || (k("isInitialized", !0), document.body.classList.add("driver-active", s("animate") ? "driver-fade" : "driver-simple"), ke(), N("overlayClick", t), N("escapePress", o), N("arrowLeftPress", f), N("arrowRightPress", w)); | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
Comment on lines
+556
to
+593
| function v(a = 0) { | ||
| var $, B, R, W, V, q, K, Y; | ||
| const p = s("steps"); | ||
| if (!p) { | ||
| console.error("No steps to drive through"), g(); | ||
| return; | ||
| } | ||
| if (!p[a]) { | ||
| g(); | ||
| return; | ||
| } | ||
| k("__activeOnDestroyed", document.activeElement), k("activeIndex", a); | ||
| const c = p[a], u = p[a + 1], h = p[a - 1], m = (($ = c.popover) == null ? void 0 : $.doneBtnText) || s("doneBtnText") || "Done", x = s("allowClose"), C = typeof ((B = c.popover) == null ? void 0 : B.showProgress) != "undefined" ? (R = c.popover) == null ? void 0 : R.showProgress : s("showProgress"), b = (((W = c.popover) == null ? void 0 : W.progressText) || s("progressText") || "{{current}} of {{total}}").replace("{{current}}", `${a + 1}`).replace("{{total}}", `${p.length}`), P = ((V = c.popover) == null ? void 0 : V.showButtons) || s("showButtons"), E = [ | ||
| "next", | ||
| "previous", | ||
| ...x ? ["close"] : [] | ||
| ].filter((ce) => !(P != null && P.length) || P.includes(ce)), T = ((q = c.popover) == null ? void 0 : q.onNextClick) || s("onNextClick"), A = ((K = c.popover) == null ? void 0 : K.onPrevClick) || s("onPrevClick"), H = ((Y = c.popover) == null ? void 0 : Y.onCloseClick) || s("onCloseClick"); | ||
| j({ | ||
| ...c, | ||
| popover: { | ||
| showButtons: E, | ||
| nextBtnText: u ? void 0 : m, | ||
| disableButtons: [...h ? [] : ["previous"]], | ||
| showProgress: C, | ||
| progressText: b, | ||
| onNextClick: T || (() => { | ||
| u ? v(a + 1) : g(); | ||
| }), | ||
| onPrevClick: A || (() => { | ||
| v(a - 1); | ||
| }), | ||
| onCloseClick: H || (() => { | ||
| g(); | ||
| }), | ||
| ...(c == null ? void 0 : c.popover) || {} | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
Comment on lines
+594
to
+619
| function g(a = !0) { | ||
| const p = l("__activeElement"), c = l("__activeStep"), u = l("__activeOnDestroyed"), h = s("onDestroyStarted"); | ||
| if (a && h) { | ||
| const C = !p || (p == null ? void 0 : p.id) === "driver-dummy-element"; | ||
| h(C ? void 0 : p, c, { | ||
| config: s(), | ||
| state: l(), | ||
| driver: _() | ||
| }); | ||
| return; | ||
| } | ||
| const m = (c == null ? void 0 : c.onDeselected) || s("onDeselected"), x = s("onDestroyed"); | ||
| if (document.body.classList.remove("driver-active", "driver-fade", "driver-simple"), _e(), Te(), Ce(), me(), de(), X(), p && c) { | ||
| const C = p.id === "driver-dummy-element"; | ||
| m && m(C ? void 0 : p, c, { | ||
| config: s(), | ||
| state: l(), | ||
| driver: _() | ||
| }), x && x(C ? void 0 : p, c, { | ||
| config: s(), | ||
| state: l(), | ||
| driver: _() | ||
| }); | ||
| } | ||
| u && u.focus(); | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.There was a problem hiding this comment.
Pull Request Overview
This PR adds support for OverlayClickBehavior to the DriverJs component, allowing developers to customize what happens when users click on the overlay during a guided tour. The implementation updates the underlying driver.js library (from .js to .mjs), adds new configuration options in C#, and implements JavaScript interop for handling custom overlay click callbacks.
Key changes:
- Updated driver.js library to support
overlayClickBehaviorwith options: "close", "nextStep", or custom function callback - Added C# configuration properties
OverlayClickBehaviorandOnOverlayClickedAsyncfor overlay click handling - Implemented JavaScript interop to bridge overlay click events between the driver.js library and Blazor component
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js.mjs | New version of driver.js library with overlay click behavior support and additional features (function-based element selection) |
| src/components/BootstrapBlazor.DriverJs/wwwroot/driver.js | Removed old version, replaced by .mjs module |
| src/components/BootstrapBlazor.DriverJs/Components/DriverJsConfig.cs | Added OverlayClickBehavior string property and OnOverlayClickedAsync callback, plus unused OverlayClickCallbackMethod property; updated copyright header |
| src/components/BootstrapBlazor.DriverJs/Components/DriverJs.razor.js | Updated import to use .mjs module, added logic to handle overlay click behavior when set to "function" |
| src/components/BootstrapBlazor.DriverJs/Components/DriverJs.razor.cs | Added OnOverlayClicked JSInvokable method to handle overlay click callbacks from JavaScript; updated copyright header and null-checking patterns |
| src/components/BootstrapBlazor.DriverJs/BootstrapBlazor.DriverJs.csproj | Incremented version to 10.0.1; removed RootNamespace property |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /// <summary> | ||
| /// 点击组件这招回调方法由 JavaScript 调用 |
There was a problem hiding this comment.
Spelling error in comment: "这招" should likely be "遮罩" (overlay/mask in Chinese).
Suggested change
| /// 点击组件这招回调方法由 JavaScript 调用 | |
| /// 点击组件遮罩回调方法由 JavaScript 调用 |
| private string OverlayClickCallbackMethod => nameof(OnOverlayClickedAsync); | ||
|
|
||
| /// <summary> | ||
| /// 获得/设置 组件销毁前回调方法名称 |
There was a problem hiding this comment.
The documentation comment "获得/设置 组件销毁前回调方法名称" (Gets/sets the method name before component destruction) is incorrect. This property is for overlay click callback, not destruction callback. The comment should describe that this is the callback method for handling overlay click events.
Suggested change
| /// 获得/设置 组件销毁前回调方法名称 | |
| /// 获得/设置 点击遮罩时的回调方法 |
Comment on lines
+152
to
+153
| [JsonInclude] | ||
| private string OverlayClickCallbackMethod => nameof(OnOverlayClickedAsync); |
There was a problem hiding this comment.
The OverlayClickCallbackMethod property doesn't appear to be used anywhere in the JavaScript interop code (DriverJs.razor.js). The JavaScript checks overlayClickBehavior === "function" directly on line 37. This property either should be removed if unused, or the JavaScript should be using it to determine which method to invoke. Consider aligning with the existing pattern used for HookDestroyStarted and HookDestroyed (lines 128-129, 137-138) which use boolean properties.
Suggested change
| [JsonInclude] | |
| private string OverlayClickCallbackMethod => nameof(OnOverlayClickedAsync); |
|
|
||
| <PropertyGroup> | ||
| <RootNamespace>BootstrapBlazor.Components</RootNamespace> | ||
| <Version>10.0.1</Version> |
There was a problem hiding this comment.
The RootNamespace property was removed in this change. All other components in the repository (e.g., BootstrapBlazor.Chart, BootstrapBlazor.Authenticator, BootstrapBlazor.BaiduOcr) maintain <RootNamespace>BootstrapBlazor.Components</RootNamespace> in their project files. Unless this removal is intentional, the property should be retained for consistency across the codebase.
Suggested change
| <Version>10.0.1</Version> | |
| <Version>10.0.1</Version> | |
| <RootNamespace>BootstrapBlazor.Components</RootNamespace> |
| } | ||
| if (overlayClickBehavior === "function") { | ||
| options.overlayClickBehavior = async (el, step, { state }) => { | ||
| await d.invoke.invokeMethodAsync("OnOverlayClicked", state.activeIndex) |
There was a problem hiding this comment.
Missing semicolon at the end of the statement. While JavaScript has automatic semicolon insertion (ASI), it's a best practice to include semicolons explicitly for consistency with the rest of the codebase (see lines 28, 34, 35, etc.).
Suggested change
| await d.invoke.invokeMethodAsync("OnOverlayClicked", state.activeIndex) | |
| await d.invoke.invokeMethodAsync("OnOverlayClicked", state.activeIndex); |
| function g(a = !0) { | ||
| const p = l("__activeElement"), c = l("__activeStep"), u = l("__activeOnDestroyed"), h = s("onDestroyStarted"); | ||
| if (a && h) { | ||
| const C = !p || (p == null ? void 0 : p.id) === "driver-dummy-element"; |
There was a problem hiding this comment.
This guard always evaluates to false.
Suggested change
| const C = !p || (p == null ? void 0 : p.id) === "driver-dummy-element"; | |
| const C = !p || (p != null && p.id === "driver-dummy-element"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #706
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Implement overlay click behavior support in DriverJs by adding a new config option, .NET callback, and updated JavaScript driver module
New Features:
Enhancements:
Chores: