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.
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
Add disable zoom on mac #3289
Add disable zoom on mac #3289
Changes from 1 commit
c49c3e5
4216cee
916c3e8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 introduction of the
zoomable
parameter and the associated logic to disable the zoom button ifzoomable
is false is a well-implemented feature that directly addresses the PR objectives. However, there are a few considerations and potential improvements:Parameter Documentation: It's important to document the new
zoomable
parameter in the method's header comment (if applicable) to ensure that other developers understand its purpose and how it affects window behavior.Consistency with macOS Guidelines: Disabling the zoom button while allowing resizing is unconventional in macOS applications. Ensure this behavior aligns with macOS Human Interface Guidelines and consider how it might affect user expectations.
Testing: The PR description mentions that unit tests for this feature have not been added due to questions about testability. While testing UI behavior can be challenging, consider exploring UI testing frameworks like XCTest for macOS that support automated UI tests. This could help ensure the feature works as expected across different macOS versions and configurations.
Fallback for Non-resizable Windows: The logic correctly disables the zoom button only if the window is resizable. Consider adding a comment or documentation noting that this feature is intended for use with resizable windows, as the behavior might be unclear if
zoomable
is set to false for a non-resizable window.Overall, the changes are aligned with the PR objectives and are a valuable addition to the Wails framework for macOS developers. Just ensure to address the considerations mentioned for documentation, testing, and adherence to macOS guidelines.
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 addition of the
DisableZoom
option to themac.Options
struct is correctly documented. However, it would be beneficial to include a brief example of how to set this option in code, similar to other options documented in this file. This would provide developers with a clear, practical example of how to use the new feature.Consider adding a code snippet demonstrating how to set the
DisableZoom
option totrue
orfalse
within themac.Options
struct.