-
Notifications
You must be signed in to change notification settings - Fork 5
947956 : Custom Toolbar Zoom Percentage is not working properly #50
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
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.
Please address the provided feedbacks
<value>2.0</value> | ||
</resheader> | ||
<resheader name="reader"> | ||
<value>System.Resources.ResXResourceReader, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value> |
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.
Check whether this is present in all other samples. If not, remove it.
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.
This present in other sample also na
} | ||
} | ||
} | ||
} |
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 below image file have many duplicates. Remove the unused images
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.
As you said i checked and removed the unused images
|
||
namespace syncfusion.pdfviewerdemos.wpf | ||
{ | ||
public class PdfViewerEventAttachUtil |
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.
Why we have added this file here? what is the usage
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.
In this class they handled the custom toolbar view model loaded and closed event
@@ -0,0 +1,6 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<Project ToolsVersion="Current" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> |
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.
Remove this .User file. Ith should not be commited
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.
As you said I removed the user file
<ItemGroup> | ||
<Reference Include="Syncfusion.Compression.Base"> | ||
<SpecificVersion>False</SpecificVersion> | ||
<HintPath>..\..\..\..\..\..\..\Assemblies\Syncfusion.Compression.Base.dll</HintPath> |
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.
Why the reference on given as paths?
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.
As you said i removed the reference path
<OutputType>WinExe</OutputType> | ||
<RootNamespace>CustomToolbar</RootNamespace> | ||
<AssemblyName>CustomToolbar</AssemblyName> | ||
<TargetFrameworkVersion>v4.6.2</TargetFrameworkVersion> |
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.
Where is the project file for .NET Core?
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.
Core projects also created na
# Visual Studio Version 17 | ||
VisualStudioVersion = 17.5.2.0 | ||
MinimumVisualStudioVersion = 10.0.40219.1 | ||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "CustomToolbar", "CustomToolbar.csproj", "{BDB4645E-EC1F-6D00-4FE7-2C6AB5166375}" |
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.
Why there is 2 solution file for a single project?
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.
As you said delete the duplicate solution na
CustomToolBar m_customToolbarWindow = null; | ||
private string m_fileName; | ||
private string m_selectedItem; | ||
private int[] zoomLevels = { 50, 75, 100, 125, 150, 200, 400 }; |
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.
Is this needed?
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.
Yes na, in pdfviewer also they declared like this na
m_customToolbarWindow.ZoomIn.IsEnabled = true; | ||
m_customToolbarWindow.ZoomOut.IsEnabled = true; | ||
if(m_customToolbarWindow.pdfviewer.ZoomPercentage >= m_customToolbarWindow.pdfviewer.MaximumZoomPercentage) | ||
m_customToolbarWindow.ZoomOut.IsEnabled = false; |
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.
Why we are making zoom out false here
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.
I checked and changed the code
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.
Please address the provided feedbacks
if (m_customToolbarWindow.pdfviewer.LoadedDocument != null) | ||
{ | ||
int currentZoom = m_customToolbarWindow.pdfviewer.ZoomPercentage; | ||
int nextZoom = currentZoom; |
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.
What is the use of this assignment?
m_customToolbarWindow.ZoomComboBox.Text = nextZoom.ToString(); | ||
m_customToolbarWindow.pdfviewer.ZoomMode = Syncfusion.Windows.PdfViewer.ZoomMode.Default; | ||
m_customToolbarWindow.FitWidth.IsEnabled = true; | ||
if (currentZoom <= zoomLevels[6]) |
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.
Don't use hardcoded checks
} | ||
} | ||
|
||
void WireUpEvents() |
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.
Add Access modifiers
Bug description
947956 - Custom Toolbar Zoom Percentage is not working properly - Known Issue
Root cause
The root cause of the bug is that in the existing implementation, Zoom in and zoom out percentage not properly updated in the zoom combobox.
Release Notes Content
Custom Toolbar Zoom Percentage is now working properly
Reason for not identifying earlier
This was missed in the requirement of the feature implementation
Is it a breaking issue?
If it is a breaking issue, provide the commit details which caused this break. - No
Action taken
Manual test case will be added.
Solution description
In the proposed solution, I have implemented logic to handle zoom in and zoom out events
Testcase details
Areas affected and ensured
Message box sentence
Additional checklist
Did you run the automation against your changes?
Did you record this case in the unit test or UI test?
No known issues. If there are known issues, mention them here.
Have you followed the coding
guidelines
?There are no API name changes. If Yes, Have you got API approval for the API review task? If yes, mention the task link here | If no, create an API review task
No framework issues in these changes. If Yes, create a bug report to them or update our side points in the existing bug report and mention the links here.
No existing behavior or UI changes of other features due to this code change. If Yes, what is that?
Did you ensure both touch and mouse interactions for this change?
Did you test the change by resizing the window randomly?
Did you ensure and make changes for all the applicable themes
theme support
?Have you ensured this change with RTL?
Have you ensured this change with
Keyboard Interactions
?Have you ensured this change with
Accessibility
?Have you ensured this change with Narrator?
Have you considered
localization
if applicable?Have you ensured these changes with the sample browser?
Did you test the
memory leak
with your fix?Did you check CPU usage for this change?
Is there any public API name change?
Is the test result uploaded in test server?
Did you manually check the source compilation in .NET 2.0 framework and .NET core projects?
Is this code change ensured in Rotation, Magnification and Redaction?
Does your code changes followed naming conventions? https://syncfusion.sharepoint.com/sites/PDFViewerDesktop/SitePages/Naming%20Conventions%20for%20the%20code.aspx
Have you ensured the fix after rebasing the outdated source?
Have you ensured validating the differences obtained in the test suite [Mention the differences count here]?