feat(UniverSheet): add LoadingText parameter #901
Merged
Conversation
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
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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Comment on lines
+117
to
+119
| const { onRendered } = sheet.events; | ||
| if (onRendered) { | ||
| const disposable = univerAPI.addEvent(univerAPI.Event.LifeCycleChanged, e => { |
There was a problem hiding this comment.
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.
Comment on lines
+5
to
+7
| <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.
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.
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.
Comment on lines
+5
to
+10
| <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> |
There was a problem hiding this comment.
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.
Suggested change
| <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 未设置读取资源文件 |
There was a problem hiding this comment.
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.
Suggested change
| /// 获得/设置 正在加载显示文本 默认 null 未设置读取资源文件 | |
| /// 获得/设置 正在加载显示文本 默认 null 未设置，读取资源文件 |
| // Website: https://www.blazor.zone or https://argozhang.github.io/ | ||
|
|
||
| using Microsoft.AspNetCore.Components; | ||
| using Microsoft.Extensions.Localization; |
There was a problem hiding this comment.
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.
Suggested change
| using Microsoft.Extensions.Localization; | |
| using Microsoft.Extensions.Localization; | |
| using System.Diagnostics.CodeAnalysis; |
| const { stage } = e; | ||
| if (stage === univerAPI.Enum.LifecycleStages.Rendered) { | ||
| onRendered(); | ||
| disposable.dispose() |
There was a problem hiding this comment.
Missing semicolon after disposable.dispose(). Add a semicolon at the end of this statement to maintain code consistency and avoid potential JavaScript issues.
Suggested change
| 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;"> |
There was a problem hiding this comment.
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.
Suggested change
| <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 => { |
There was a problem hiding this comment.
Unused variable loadAssets2.
Comment on lines
47
to
48
| const lang = sheet.lang.replace('-', '') | ||
| const langStr = lang.charAt(0).toUpperCase() + lang.slice(1) |
There was a problem hiding this comment.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
Suggested change
| 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) |
There was a problem hiding this comment.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #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: