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

#203 Improve pattern support #602

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

mnndnl
Copy link
Contributor

@mnndnl mnndnl commented Jun 11, 2019

No description provided.

@ystrot ystrot self-assigned this Jun 14, 2019
@ystrot ystrot added this to the 0.9.6 milestone Jun 14, 2019
} catch {
XCTFail(error.localizedDescription)
return
print(error.localizedDescription)
Copy link
Member

Choose a reason for hiding this comment

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

Why XCTFail replaced with print?

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, let's separate pattern support and tests improvements.

private let testFolderName = "MacawTestOutputData"
private let shouldComparePNGImages = true
private let multipleTestsWillRun = false
private let shouldSaveFaildedTestImage = false
Copy link
Member

Choose a reason for hiding this comment

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

You have removed few options in tests. For example, there is no shouldSaveFaildedTestImage which means that tests can generate some data which is not a good default value.

I would recommend to remove all these changes from this PR to keep it only about pattern support and create another one especially for tests improvements where we can discuss these changes.

public let content: Node
public let bounds: Rect
public let userSpace: Bool
public let position: Transform
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename position to place.


public init(content: Node, bounds: Rect, userSpace: Bool = false) {
public init(content: Node, bounds: Rect, viewBox: Rect, userSpace: Bool = false, position: Transform) {
Copy link
Member

Choose a reason for hiding this comment

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

You need to keep in mind that this is a breaking change, because Pattern is a part of public API. New attributes should always have default value. I'm also not quite sure that viewBox should have separate attribute, need to think about it.

@@ -150,7 +150,7 @@ open class Image: Node {
}

// Base64 image
let decodableFormat = ["image/png", "image/jpg", "image/svg+xml"]
let decodableFormat = ["image/png", "image/jpg", "image/jpeg"]
Copy link
Member

Choose a reason for hiding this comment

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

Do we avoid support for svg images here?

@mnndnl mnndnl force-pushed the fix/203-Improve-pattern-support branch from 71ea0da to c7ff33a Compare June 14, 2019 09:34
@ystrot ystrot modified the milestones: 0.9.6, 0.9.7 Apr 10, 2020
@ystrot ystrot modified the milestones: 0.9.7, 0.9.x Jul 28, 2020
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.

2 participants