-
-
Notifications
You must be signed in to change notification settings - Fork 6
feat(UniverSheet): add LoadingText parameter #901
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
Conversation
Reviewer's GuideIntroduces a configurable loading overlay for UniverSheet with a new LoadingText parameter, wiring it through the Blazor component, JS interop, and localized resources, and adjusts the DOM structure and styling to support showing and hiding the loading backdrop when rendering completes. Sequence diagram for UniverSheet initialization and loading overlay hidesequenceDiagram
actor User
participant Blazor as BlazorApp
participant Component as UniverSheetComponent
participant Dom as BrowserDOM
participant JSInterop as UniverSheetJsInterop
participant SheetConfig as UniverSheetClientConfig
participant Univer as UniverSheetJsModule
participant API as UniverAPI
User->>Blazor: Navigate to page with UniverSheet
Blazor->>Component: Instantiate UniverSheet
Component->>Component: OnParametersSet()
Component->>Component: Lang ??= CurrentUICulture
Component->>Component: LoadingText ??= Localizer[LoadingText]
Component->>Dom: Render markup
Note right of Dom: Root div with<br/>bb_univer_sheet_wrap and<br/>bb_univer_sheet_backdrop
Blazor->>JSInterop: init(id, invoke, options)
JSInterop->>Dom: const el = document.getElementById(id)
JSInterop->>SheetConfig: Create config with<br/>el = el.querySelector(bb-univer-sheet-wrap)<br/>events.onRendered = hideBackdrop
JSInterop->>Univer: createUniverSheetAsync(SheetConfig)
Univer->>Univer: loadAssets(lang)
Univer->>API: configure universheet and events
Univer->>API: addEvent(LifeCycleChanged, handler)
API-->>Univer: LifeCycleChanged(stage = Rendered)
Univer->>SheetConfig: handler(stage)
SheetConfig->>SheetConfig: onRendered()
SheetConfig->>Dom: backdrop.classList.add(d-none)
Dom-->>User: Sheet visible without loading overlay
Class diagram for updated UniverSheet component and JS interopclassDiagram
class UniverSheet {
+string? Lang
+UniverSheetData? Data
+string? LoadingText
+Func~UniverSheetData?, Task~UniverSheetData?~~? OnPostDataAsync
-IStringLocalizer~UniverSheet~ Localizer
+void OnParametersSet()
}
class UniverSheetRazorMarkup {
+div rootContainer
+div bb_univer_sheet_wrap
+div bb_univer_sheet_backdrop
+string LoadingText
}
class UniverSheetJsInterop {
+Task init(string id, DotNetObjectReference invoke, UniverSheetOptions options)
}
class UniverSheetOptions {
+string theme
+string lang
+object plugins
+UniverSheetData data
+string ribbonType
+bool darkMode
}
class UniverSheetClientConfig {
+HTMLElement el
+DotNetObjectReference invoke
+UniverSheetData data
+object plugins
+string theme
+string lang
+string ribbonType
+bool darkMode
+UniverSheetEvents events
}
class UniverSheetEvents {
+Action onRendered()
}
class UniverSheetJsModule {
+Task createUniverSheetAsync(UniverSheetClientConfig sheet)
}
class UniverAPI {
+EventSource Event
+EnumSource Enum
+Disposable addEvent(string eventType, Func~LifeCycleEvent, void~ handler)
}
class LifeCycleEvent {
+LifecycleStages stage
}
class LifecycleStages {
<<enumeration>>
Rendered
}
UniverSheet --> UniverSheetRazorMarkup : rendered_as
UniverSheet --> UniverSheetJsInterop : uses
UniverSheet --> UniverSheetOptions : builds
UniverSheetJsInterop --> UniverSheetClientConfig : constructs
UniverSheetClientConfig --> UniverSheetEvents : has
UniverSheetJsInterop --> UniverSheetJsModule : calls
UniverSheetJsModule --> UniverAPI : configures
UniverAPI --> LifeCycleEvent : raises
LifeCycleEvent --> LifecycleStages : uses
UniverSheet --> IStringLocalizer~UniverSheet~ : injects
UniverSheetRazorMarkup --> LoadingText : displays_backdrop_text
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 2 issues, and left some high level feedback:
- Consider moving the inline styles for the wrapper and backdrop in
UniverSheet.razorinto CSS classes so layout and visual behavior are centralized and easier to maintain or override. - When defaulting
LoadingTextwithLocalizer[nameof(LoadingText)], consider adding a fallback (e.g., to a hard-coded string) so the UI doesn’t show the resource key if a corresponding localization entry is missing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving the inline styles for the wrapper and backdrop in `UniverSheet.razor` into CSS classes so layout and visual behavior are centralized and easier to maintain or override.
- When defaulting `LoadingText` with `Localizer[nameof(LoadingText)]`, consider adding a fallback (e.g., to a hard-coded string) so the UI doesn’t show the resource key if a corresponding localization entry is missing.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.UniverSheet/wwwroot/univer.js:117-119` </location>
<code_context>
univerAPI.createWorkbook();
}
+ const { onRendered } = sheet.events;
+ if (onRendered) {
+ const disposable = univerAPI.addEvent(univerAPI.Event.LifeCycleChanged, e => {
+ const { stage } = e;
+ if (stage === univerAPI.Enum.LifecycleStages.Rendered) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `sheet.events` being undefined before destructuring.
If callers omit `sheet.events`, `const { onRendered } = sheet.events;` will throw at runtime. Consider using optional chaining (e.g. `const onRendered = sheet.events?.onRendered;`) or ensuring `sheet.events` is defaulted when constructing `sheet` to handle partial/legacy callers safely.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.UniverSheet/Components/UniverSheet.razor:5-7` </location>
<code_context>
@attribute [JSModuleAutoLoader("./_content/BootstrapBlazor.UniverSheet/Components/UniverSheet.razor.js", JSObjectReference = true)]
-<div @attributes="@AdditionalAttributes" class="@ClassString" id="@Id"></div>
+<div @attributes="@AdditionalAttributes" class="@ClassString" id="@Id" style="position: relative; overflow: hidden;">
+ <div class="bb-univer-sheet-wrap" style="height: 100%;"></div>
+ <div class="bb-univer-sheet-backdrop" style="background-color: #000; opacity: 0.3; position: absolute; inset: 0; z-index: 1205; display: flex; align-items: center; justify-content: center;">
+ <div style="color: #fff;">@LoadingText</div>
+ </div>
</code_context>
<issue_to_address>
**suggestion:** Consider moving inline styles to CSS to keep layout/styling consistent and maintainable.
These elements now depend on complex inline styles for positioning and centering. Moving these into CSS classes (e.g., extending `.bb-univer-sheet` and defining `.bb-univer-sheet-backdrop`) will simplify future layout/theming changes and prevent clashes with user inline styles from `AdditionalAttributes`.
Suggested implementation:
```
<div @attributes="@AdditionalAttributes" class="@($"{ClassString} bb-univer-sheet")" id="@Id">
<div class="bb-univer-sheet-wrap"></div>
<div class="bb-univer-sheet-backdrop">
<div class="bb-univer-sheet-loading-text">@LoadingText</div>
</div>
</div>
```
To complete the refactor away from inline styles, add CSS for these classes in the appropriate stylesheet (e.g. `UniverSheet.razor.css`, the library's main `.css`, or a shared theme file):
```css
.bb-univer-sheet {
position: relative;
overflow: hidden;
}
.bb-univer-sheet-wrap {
height: 100%;
}
.bb-univer-sheet-backdrop {
background-color: #000;
opacity: 0.3;
position: absolute;
inset: 0;
z-index: 1205;
display: flex;
align-items: center;
justify-content: center;
}
.bb-univer-sheet-loading-text {
color: #fff;
}
```
If `ClassString` already includes `bb-univer-sheet`, adjust the `class` attribute to avoid duplication (e.g. via the existing class-composition helper used elsewhere in the codebase).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const { onRendered } = sheet.events; | ||
| if (onRendered) { | ||
| const disposable = univerAPI.addEvent(univerAPI.Event.LifeCycleChanged, e => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Guard against sheet.events being undefined before destructuring.
If callers omit sheet.events, const { onRendered } = sheet.events; will throw at runtime. Consider using optional chaining (e.g. const onRendered = sheet.events?.onRendered;) or ensuring sheet.events is defaulted when constructing sheet to handle partial/legacy callers safely.
| <div @attributes="@AdditionalAttributes" class="@ClassString" id="@Id" style="position: relative; overflow: hidden;"> | ||
| <div class="bb-univer-sheet-wrap" style="height: 100%;"></div> | ||
| <div class="bb-univer-sheet-backdrop" style="background-color: #000; opacity: 0.3; position: absolute; inset: 0; z-index: 1205; display: flex; align-items: center; justify-content: center;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider moving inline styles to CSS to keep layout/styling consistent and maintainable.
These elements now depend on complex inline styles for positioning and centering. Moving these into CSS classes (e.g., extending .bb-univer-sheet and defining .bb-univer-sheet-backdrop) will simplify future layout/theming changes and prevent clashes with user inline styles from AdditionalAttributes.
Suggested implementation:
<div @attributes="@AdditionalAttributes" class="@($"{ClassString} bb-univer-sheet")" id="@Id">
<div class="bb-univer-sheet-wrap"></div>
<div class="bb-univer-sheet-backdrop">
<div class="bb-univer-sheet-loading-text">@LoadingText</div>
</div>
</div>
To complete the refactor away from inline styles, add CSS for these classes in the appropriate stylesheet (e.g. UniverSheet.razor.css, the library's main .css, or a shared theme file):
.bb-univer-sheet {
position: relative;
overflow: hidden;
}
.bb-univer-sheet-wrap {
height: 100%;
}
.bb-univer-sheet-backdrop {
background-color: #000;
opacity: 0.3;
position: absolute;
inset: 0;
z-index: 1205;
display: flex;
align-items: center;
justify-content: center;
}
.bb-univer-sheet-loading-text {
color: #fff;
}If ClassString already includes bb-univer-sheet, adjust the class attribute to avoid duplication (e.g. via the existing class-composition helper used elsewhere in the codebase).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a LoadingText parameter to the UniverSheet component to display customizable loading text while the sheet is initializing. The component now includes localization support for both English and Chinese.
Changes:
- Added a new
LoadingTextparameter with localization support (English and Chinese resource files) - Implemented a loading backdrop that displays during sheet initialization and is hidden when rendering is complete
- Restructured the component's HTML to support the loading overlay
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
univer-sheet.bundle.css |
Removed overflow: hidden from CSS class (moved to inline style) |
univer.js |
Added event listener for Rendered lifecycle stage to trigger onRendered callback, improved code formatting |
zh.json |
Added Chinese localization for LoadingText ("正在加载 ...") |
en.json |
Added English localization for LoadingText ("Loading ...") |
UniverSheet.razor.js |
Modified element selection and added onRendered event to hide loading backdrop |
UniverSheet.razor.cs |
Added LoadingText parameter with localization support and injected IStringLocalizer |
UniverSheet.razor |
Added HTML structure for loading backdrop with loading text display |
BootstrapBlazor.UniverSheet.csproj |
Version bump to 10.0.7 and configured localization files as embedded resources |
Comments suppressed due to low confidence (2)
src/components/BootstrapBlazor.UniverSheet/wwwroot/univer.js:38
- Unused variable LocaleType.
const { LocaleType, merge } = UniverCore;
src/components/BootstrapBlazor.UniverSheet/wwwroot/univer.js:132
- Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div @attributes="@AdditionalAttributes" class="@ClassString" id="@Id" style="position: relative; overflow: hidden;"> | ||
| <div class="bb-univer-sheet-wrap" style="height: 100%;"></div> | ||
| <div class="bb-univer-sheet-backdrop" style="background-color: #000; opacity: 0.3; position: absolute; inset: 0; z-index: 1205; display: flex; align-items: center; justify-content: center;"> | ||
| <div style="color: #fff;">@LoadingText</div> | ||
| </div> | ||
| </div> |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline styles should be extracted to a CSS class for better maintainability. The backdrop styling on line 7 and the wrapper on line 6 contain multiple inline styles that would be better managed in a stylesheet.
| <div @attributes="@AdditionalAttributes" class="@ClassString" id="@Id" style="position: relative; overflow: hidden;"> | |
| <div class="bb-univer-sheet-wrap" style="height: 100%;"></div> | |
| <div class="bb-univer-sheet-backdrop" style="background-color: #000; opacity: 0.3; position: absolute; inset: 0; z-index: 1205; display: flex; align-items: center; justify-content: center;"> | |
| <div style="color: #fff;">@LoadingText</div> | |
| </div> | |
| </div> | |
| <div @attributes="@AdditionalAttributes" class="@ClassString bb-univer-sheet-root" id="@Id"> | |
| <div class="bb-univer-sheet-wrap bb-univer-sheet-wrap-fullheight"></div> | |
| <div class="bb-univer-sheet-backdrop bb-univer-sheet-backdrop-style"> | |
| <div class="bb-univer-sheet-loading-text">@LoadingText</div> | |
| </div> | |
| </div> | |
| <style> | |
| .bb-univer-sheet-root { | |
| position: relative; | |
| overflow: hidden; | |
| } | |
| .bb-univer-sheet-wrap-fullheight { | |
| height: 100%; | |
| } | |
| .bb-univer-sheet-backdrop-style { | |
| background-color: #000; | |
| opacity: 0.3; | |
| position: absolute; | |
| inset: 0; | |
| z-index: 1205; | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| } | |
| .bb-univer-sheet-loading-text { | |
| color: #fff; | |
| } | |
| </style> |
| public UniverSheetData? Data { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// 获得/设置 正在加载显示文本 默认 null 未设置读取资源文件 |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XML documentation comment is in Chinese. The comment states "获得/设置 正在加载显示文本 默认 null 未设置读取资源文件" which appears to be missing punctuation and spacing. It should read: "获得/设置 正在加载显示文本 默认 null 未设置,读取资源文件" with a comma added before "读取资源文件" for proper grammar.
| /// 获得/设置 正在加载显示文本 默认 null 未设置读取资源文件 | |
| /// 获得/设置 正在加载显示文本 默认 null 未设置,读取资源文件 |
| // Website: https://www.blazor.zone or https://argozhang.github.io/ | ||
|
|
||
| using Microsoft.AspNetCore.Components; | ||
| using Microsoft.Extensions.Localization; |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NotNull attribute is used on line 64, but the required namespace System.Diagnostics.CodeAnalysis is not imported. Add using System.Diagnostics.CodeAnalysis; to the using statements to resolve this compilation issue.
| using Microsoft.Extensions.Localization; | |
| using Microsoft.Extensions.Localization; | |
| using System.Diagnostics.CodeAnalysis; |
| const { stage } = e; | ||
| if (stage === univerAPI.Enum.LifecycleStages.Rendered) { | ||
| onRendered(); | ||
| disposable.dispose() |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon after disposable.dispose(). Add a semicolon at the end of this statement to maintain code consistency and avoid potential JavaScript issues.
| disposable.dispose() | |
| disposable.dispose(); |
| <div @attributes="@AdditionalAttributes" class="@ClassString" id="@Id"></div> | ||
| <div @attributes="@AdditionalAttributes" class="@ClassString" id="@Id" style="position: relative; overflow: hidden;"> | ||
| <div class="bb-univer-sheet-wrap" style="height: 100%;"></div> | ||
| <div class="bb-univer-sheet-backdrop" style="background-color: #000; opacity: 0.3; position: absolute; inset: 0; z-index: 1205; display: flex; align-items: center; justify-content: center;"> |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loading backdrop lacks accessibility attributes. Consider adding role="status" and aria-live="polite" to the backdrop div to inform screen reader users that content is loading.
| <div class="bb-univer-sheet-backdrop" style="background-color: #000; opacity: 0.3; position: absolute; inset: 0; z-index: 1205; display: flex; align-items: center; justify-content: center;"> | |
| <div class="bb-univer-sheet-backdrop" role="status" aria-live="polite" style="background-color: #000; opacity: 0.3; position: absolute; inset: 0; z-index: 1205; display: flex; align-items: center; justify-content: center;"> |
| import { addScript, addLink, getTheme } from '../BootstrapBlazor/modules/utility.js' | ||
| import DataService from './data-service.js' | ||
|
|
||
| const loadAssets2 = async lang => { |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable loadAssets2.
| const lang = sheet.lang.replace('-', '') | ||
| const langStr = lang.charAt(0).toUpperCase() + lang.slice(1) |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
| const lang = sheet.lang.replace('-', '') | |
| const langStr = lang.charAt(0).toUpperCase() + lang.slice(1) | |
| const lang = sheet.lang.replace('-', ''); | |
| const langStr = lang.charAt(0).toUpperCase() + lang.slice(1); |
| const { UniverSheetsDataValidationPreset } = UniverPresetSheetsDataValidation; | ||
|
|
||
| const lang = sheet.lang.replace('-', '') | ||
| const langStr = lang.charAt(0).toUpperCase() + lang.slice(1) |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
Link issues
fixes #900
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add configurable loading text and backdrop overlay for UniverSheet while the workbook is rendering, and wire sheet lifecycle events to hide the loading state once rendering completes.
New Features:
Enhancements: