feat(PdfViewer): add ShowToolbar parameter #913
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 GuideAdds a configurable ShowToolbar parameter to PdfViewer that controls PDF URL fragments for toolbar and thumbnail sidebar visibility, while refactoring URL fragment construction and enhancing XML documentation with bilingual comments. Sequence diagram for PdfViewer URL generation with ShowToolbarsequenceDiagram
participant PdfViewer
participant NavigationManager
participant UriBuilder
PdfViewer->>NavigationManager: ToAbsoluteUri(Url)
NavigationManager-->>PdfViewer: uri
PdfViewer->>UriBuilder: create UriBuilder(uri.AbsoluteUri)
PdfViewer->>PdfViewer: BuildFragment(builder)
PdfViewer->>UriBuilder: set Fragment(BuildFragment)
UriBuilder-->>PdfViewer: Uri.ToString() with_page_and_toolbar_fragments
Class diagram for updated PdfViewer with ShowToolbarclassDiagram
class PdfViewer {
string Url
int PageIndex
string Height
Func~Task~ OnLoaded
Func~Task~ NotSupportCallback
bool UseGoogleDocs
bool ShowToolbar
Task OnAfterRenderAsync(bool firstRender)
Task InvokeInitAsync()
string GetAbsoluteUri(string url)
string BuildFragment(UriBuilder builder)
Task TriggerOnLoaded()
Task TriggerNotSupportCallback()
}
class NavigationManager {
Uri ToAbsoluteUri(string url)
}
PdfViewer --> NavigationManager : uses
File-Level Changes
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 1 issue, and left some high level feedback:
- In
BuildFragment, theUriBuilder builderparameter is never used, so it can be removed (and callers updated) to simplify the method signature and avoid confusion. - Consider extracting the fragment construction constants (e.g.,
"toolbar=0&navpanes=0") into named constants or separate helpers to make the intent clearer and reduce the chance of typos if reused or extended later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `BuildFragment`, the `UriBuilder builder` parameter is never used, so it can be removed (and callers updated) to simplify the method signature and avoid confusion.
- Consider extracting the fragment construction constants (e.g., `"toolbar=0&navpanes=0"`) into named constants or separate helpers to make the intent clearer and reduce the chance of typos if reused or extended later.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfViewer/PdfViewer.razor.cs:97` </location>
<code_context>
}
var uri = NavigationManager.ToAbsoluteUri(url);
- return $"{uri.AbsoluteUri}#page={PageIndex}";
+ var builder = new UriBuilder(uri.AbsoluteUri);
+ builder.Fragment = BuildFragment(builder);
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new URI construction logic in `GetAbsoluteUri` to avoid the extra `UriBuilder` and `BuildFragment` indirection while still supporting `ShowToolbar` and page index behavior.
The new `GetAbsoluteUri` / `BuildFragment` flow adds indirection without clear benefit. You can keep the new `ShowToolbar` behavior while simplifying the URI construction and removing `UriBuilder` and the extra method.
A more direct implementation that preserves current behavior (including overwriting any existing fragment and using `"toolbar=0&navpanes=0"` as a single unit) could be:
```csharp
private string GetAbsoluteUri(string? url)
{
if (string.IsNullOrEmpty(url))
{
return string.Empty;
}
var baseUri = NavigationManager.ToAbsoluteUri(url).AbsoluteUri;
string? fragment = null;
if (PageIndex > 0)
{
fragment = $"page={PageIndex}";
}
if (!ShowToolbar)
{
var toolbarFragment = "toolbar=0&navpanes=0";
fragment = string.IsNullOrEmpty(fragment)
? toolbarFragment
: $"{fragment}&{toolbarFragment}";
}
return string.IsNullOrEmpty(fragment)
? baseUri
: $"{baseUri}#{fragment}";
}
```
This:
- Keeps all existing functionality (page index, optional toolbar/navpanes hiding).
- Matches the current behavior of replacing any existing fragment.
- Removes the extra `UriBuilder` allocation and `BuildFragment` method.
- Avoids the `List<string>` / `string.Join` overhead for at most two conditions, making the control flow easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| var uri = NavigationManager.ToAbsoluteUri(url); | ||
| return $"{uri.AbsoluteUri}#page={PageIndex}"; |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the new URI construction logic in GetAbsoluteUri to avoid the extra UriBuilder and BuildFragment indirection while still supporting ShowToolbar and page index behavior.
The new GetAbsoluteUri / BuildFragment flow adds indirection without clear benefit. You can keep the new ShowToolbar behavior while simplifying the URI construction and removing UriBuilder and the extra method.
A more direct implementation that preserves current behavior (including overwriting any existing fragment and using "toolbar=0&navpanes=0" as a single unit) could be:
private string GetAbsoluteUri(string? url)
{
if (string.IsNullOrEmpty(url))
{
return string.Empty;
}
var baseUri = NavigationManager.ToAbsoluteUri(url).AbsoluteUri;
string? fragment = null;
if (PageIndex > 0)
{
fragment = $"page={PageIndex}";
}
if (!ShowToolbar)
{
var toolbarFragment = "toolbar=0&navpanes=0";
fragment = string.IsNullOrEmpty(fragment)
? toolbarFragment
: $"{fragment}&{toolbarFragment}";
}
return string.IsNullOrEmpty(fragment)
? baseUri
: $"{baseUri}#{fragment}";
}This:
- Keeps all existing functionality (page index, optional toolbar/navpanes hiding).
- Matches the current behavior of replacing any existing fragment.
- Removes the extra
UriBuilderallocation andBuildFragmentmethod. - Avoids the
List<string>/string.Joinoverhead for at most two conditions, making the control flow easier to follow.
There was a problem hiding this comment.
Pull request overview
This PR adds a ShowToolbar parameter to the PdfViewer component to control the visibility of the PDF viewer's toolbar and thumbnail sidebar. The parameter defaults to true to maintain backward compatibility.
Changes:
- Added a new boolean
ShowToolbarparameter with a default value oftrue - Updated all XML documentation comments to use bilingual format (Chinese and English)
- Refactored URL fragment building logic to support multiple PDF open parameters
- Bumped package version from implicit to 10.0.1
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| PdfViewer.razor.cs | Added ShowToolbar parameter, updated documentation to bilingual format, refactored fragment building with new BuildFragment method, and removed empty <returns> tags |
| BootstrapBlazor.PdfViewer.csproj | Added explicit version property set to 10.0.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+114
to
+125
| private string BuildFragment(UriBuilder builder) | ||
| { | ||
| var fragments = new List<string>(); | ||
| if (PageIndex > 0) | ||
| { | ||
| fragments.Add($"page={PageIndex}"); | ||
| } | ||
| if (!ShowToolbar) | ||
| { | ||
| fragments.Add("toolbar=0&navpanes=0"); | ||
| } | ||
| return string.Join('&', fragments); |
There was a problem hiding this comment.
The builder parameter is passed to BuildFragment but never used. If the input URL already contains a fragment (e.g., "file.pdf#existingfragment"), it will be silently overwritten. If this is intentional behavior, the parameter should be removed. If existing fragments should be preserved or merged, the builder.Fragment should be checked and incorporated into the result.
| } | ||
| if (!ShowToolbar) | ||
| { | ||
| fragments.Add("toolbar=0&navpanes=0"); |
There was a problem hiding this comment.
Consider splitting "toolbar=0&navpanes=0" into two separate fragment entries: fragments.Add("toolbar=0") and fragments.Add("navpanes=0"). While the current implementation works correctly, splitting them improves maintainability and makes the code more consistent - each fragment entry should represent a single parameter rather than pre-joined parameters.
Suggested change
| fragments.Add("toolbar=0&navpanes=0"); | |
| fragments.Add("toolbar=0"); | |
| fragments.Add("navpanes=0"); |
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 #912
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add configurability to PdfViewer URL generation to optionally hide the embedded PDF toolbar and thumbnail sidebar, and improve documentation comments for its parameters.
New Features:
Enhancements: