-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
#1057 expose ZoomFactor get/set and add the respective windows only options #1463
Conversation
Thanks for this! Tests are super hard and time consuming, but if you'd like to get the ball rolling I'm open to that! What did you have in mind? |
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.
Thanks or doing this. It's looking good! Just a couple of comments/questions. The only thing that would be good to add is the docs to the options.mdx file in the website
directory 👍
v2/cmd/wails/internal/commands/generate/template/base/main.tmpl.go
Outdated
Show resolved
Hide resolved
v2/internal/frontend/desktop/windows/go-webview2/pkg/edge/ICoreWebView2Controller.go
Outdated
Show resolved
Hide resolved
The easy part are the wails specific parts, generations etc. As they are almost all self contained. Unit tests for each functions are relatively straightforward for most. If not, we may need to refactor some to allow easy testing of each specific function (ie. if a function does too many things, can be splitted in separate ones without changing the public APIs). For the integration tests (all OSes system APIs, be webviews or other), kind of tricky. Maybe writing fake functions for the C APIs. For COM, not sure, writing a fake COM sounds cumbersome. I need to think about that part :) |
v2/internal/frontend/desktop/windows/go-webview2/pkg/edge/ICoreWebView2Controller.go
Outdated
Show resolved
Hide resolved
Thanks for the great feedback and suggestion :) I will see which part I can add tests in the next couple of days. |
|
||
|
||
if opts := f.frontendOptions.Windows; opts != nil { | ||
if opts.ZoomFactor > 0.0 { |
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.
Should this be opts.ZoomFactor > 0.0 && opts.ZoomFactor != 1.0
to ignore when the default value is set?
Any updates on this @pierrejoye? I wouldn't want this blocked because of trying to work out the hard problem of testing 👍 Let me know how we can move it forward 👍 |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@pierrejoye any news on this? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
hello :)
sorry for the late reply, I did not have the time to work on the test.
that patch is used in prod here since I made the PR though :)
…On Sat, Aug 13, 2022, 3:16 PM Lea Anthony ***@***.***> wrote:
@pierrejoye <https://github.com/pierrejoye> any news on this?
—
Reply to this email directly, view it on GitHub
<#1463 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACE6KDHF3YQ6Z2C2EDNYMDVY5KVXANCNFSM5ZEFPFZQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hey there 👋 We'll look at this soon 👍 |
# Conflicts: # website/docs/reference/options.mdx
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.
LGTM 👍 , thanks for this.
As discussed in the ticket. Here is a 1st version.
Do we have tests in general? I can add the relevant ones if any needed here (while not sure how to test the underlying COM APIs calls :)