Skip to content
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

Fix #1196: Add svg support for app/window icon #2671

Merged
merged 3 commits into from Nov 30, 2021

Conversation

ChandanChainani
Copy link
Contributor

@ChandanChainani ChandanChainani commented Nov 30, 2021

Description:

Fixes #1196

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Public APIs match existing style.
  • Any breaking changes have a deprecation path or have been discussed.
  • Updated the vendor folder (using go mod vendor).

@Jacalz Jacalz changed the title FIX #1196: Can't set an app/window icon to be an svg Fix #1196: Add svg support for app/window icon Nov 30, 2021
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good. Just one suggestion to make a comment a bit more readable.

@@ -201,7 +201,8 @@ func isFileSVG(path string) bool {
return strings.ToLower(filepath.Ext(path)) == ".svg"
}

func isResourceSVG(res fyne.Resource) bool {
// IsResourceSVG checks if given fyne Resource is svg or not
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// IsResourceSVG checks if given fyne Resource is svg or not
// IsResourceSVG checks if the resource is an SVG or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jacalz Sorry for the delay done changes as suggested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to apologise ;)

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. Looks good to me :)

@andydotxyz
Copy link
Member

Great result thanks. I updated the description above because "Fixes" should just list the bug resolved, not all related PRs as well.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nice fix

@Jacalz Jacalz merged commit 8cd4a92 into fyne-io:develop Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants