feat(PdfViewer): add PdfViewer component#450
Merged
Conversation
Reviewer's GuideThis PR introduces a new PdfViewer component by adding a dedicated project, implementing server-side rendering logic in C#, integrating a JS module for client-side PDF loading, and supplying accompanying CSS and Razor markup. Sequence Diagram for PdfViewer Initialization and Initial PDF LoadsequenceDiagram
participant LC as "Component Lifecycle"
participant CSharp as "PdfViewer.razor.cs"
participant JS as "PdfViewer.razor.js"
participant DOM as "Browser DOM"
LC->>+CSharp: Initialize Component
CSharp->>CSharp: InvokeInitAsync()
CSharp->>JS: Invoke JS: init(Id)
activate JS
JS->>JS: addLink("./_content/BootstrapBlazor.PdfViewer/pdf-viewer.css")
JS->>DOM: Get element by Id
JS->>DOM: Get 'data-bb-url' attribute (url)
JS->>JS: loadPdf(Id, url)
activate JS
alt url is provided
JS->>DOM: createFrame()
DOM-->>JS: iframeElement
JS->>DOM: el.appendChild(iframeElement)
JS->>DOM: iframeElement.src = url (PDF renders)
else url is not provided
JS->>DOM: el.innerHTML = '' (Clear content)
end
deactivate JS
deactivate JS
Sequence Diagram for PdfViewer URL UpdatesequenceDiagram
participant UserAction as "User/Application Action"
participant CSharp as "PdfViewer.razor.cs"
participant JS as "PdfViewer.razor.js"
participant IFrame as "Browser DOM (iframe)"
UserAction->>+CSharp: Update 'Url' Parameter for PdfViewer
CSharp->>CSharp: OnAfterRenderAsync(firstRender=false)
alt Url has changed (_url != Url)
CSharp->>CSharp: _url = new Url
CSharp->>JS: Invoke JS: loadPdf(Id, newUrl)
activate JS
JS->>IFrame: Get existing or create new iframe
JS->>IFrame: iframe.src = newUrl (PDF re-renders)
deactivate JS
end
deactivate CSharp
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 @ArgoZhang - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
| @inherits BootstrapModuleComponentBase | ||
| @attribute [JSModuleAutoLoader("./_content/BootstrapBlazor.PdfViewer/PdfViewer.razor.js", JSObjectReference = true, AutoInvokeDispose = false)] | ||
|
|
||
| <div @attributes="AdditionalAttributes" id="@Id" data-bb-url="@Url" class="@ClassString" style="@StyleString"></div> |
There was a problem hiding this comment.
suggestion: Merge user-defined inline styles with component-generated styles
Extract the style attribute from AdditionalAttributes and merge it with StyleString to ensure user-defined styles are preserved.
Comment on lines
+20
to
+21
| const viewer = frame || createFrame(el); | ||
| viewer.src = url; |
There was a problem hiding this comment.
issue (bug_risk): Cache the iframe reference in pdfViewer
Assign the new iframe to pdfViewer.frame to avoid creating multiple iframes when updating the URL.
| loadPdf(id, url); | ||
| } | ||
|
|
||
| export function loadPdf(id, url) { |
There was a problem hiding this comment.
suggestion (bug_risk): Guard against missing pdfViewer in Data
Check if pdfViewer is defined before destructuring to avoid runtime errors.
Comment on lines
+6
to
+8
| .bb-pdf-viewer { | ||
| width: 100%; | ||
| height: 100%; |
There was a problem hiding this comment.
suggestion: Consider adding border: none; to iframe CSS
This will prevent default borders from appearing in some browsers, ensuring a consistent, borderless PDF viewer.
| /// <inheritdoc/> | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| protected override Task InvokeInitAsync() => InvokeVoidAsync("init", Id); |
There was a problem hiding this comment.
suggestion (bug_risk): Implement disposal to clean up resources
Override DisposeAsync to remove the injected CSS link, clear the container, and delete the entry from Data to prevent memory leaks.
Suggested implementation:
/// <summary>
/// <inheritdoc/>
/// </summary>
/// <returns></returns>
protected override Task InvokeInitAsync() => InvokeVoidAsync("init", Id);
/// <inheritdoc/>
public override async ValueTask DisposeAsync()
{
// Remove the injected CSS link
await InvokeVoidAsync("removePdfViewerCss", Id);
// Clear the container
await InvokeVoidAsync("clearPdfViewerContainer", Id);
// Remove the entry from Data to prevent memory leaks
Data.Remove(Id);
await base.DisposeAsync();
}
}- You must implement the corresponding JavaScript functions
removePdfViewerCssandclearPdfViewerContainerif they do not already exist. - Ensure that
Datais accessible and is the correct dictionary holding component state; adjust the removal logic if needed. - If the base class does not have an async Dispose pattern, adjust to match the correct disposal pattern for your component base.
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 #449
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a new PdfViewer component to display PDF files in Blazor applications, including the necessary JS module, CSS assets, and project registration in the solution.
New Features: