-
Notifications
You must be signed in to change notification settings - Fork 106
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
Block macos fix #148
Block macos fix #148
Conversation
Generated by 🚫 Danger |
Sources/Core/API/Block.swift
Outdated
@@ -119,16 +119,35 @@ public final class Block: SceneObject, InstanceHashable, ActionPerformer, ZIndex | |||
layer.addSublayer(segmentLayers.top) | |||
layer.addSublayer(segmentLayers.bottom) | |||
|
|||
#if os(macOS) | |||
segmentLayers.bottom.frame.origin.y = 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.
Ah, of course! 🤦♂️ Great that you found this @mattiashagstrand 👍
Sources/Core/API/Block.swift
Outdated
|
||
let centerReplicatorLayer = ReplicatorLayer() | ||
#if os(macOS) |
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.
All this conditional compilation makes this code really hard to follow IMO (it was already a bit hard to follow, and that's my fault 😅), how about breaking it up into some helper functions and put the conditional compilation inside those functions (will make it a bit more contained)?
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.
Absolutely agree with you @JohnSundell, I fell for the old "it's already a bit complicated so what does it matter if I make it a little bit more complicated". My bad! I will take another look and see if I can break up the code a little. It is a little risky since we don't have that many unit tests on Block and it isn't that easy to add either. But at least you get clear visual feedback if it is wrong... 😄
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.
Haha yeah, it's a bit tricky to test these purely visual things 🤔 We could always verify the layer hierarchy but I'm not sure if that makes a ton of sense.
@JohnSundell, do you think this refactoring is enough? |
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.
Awesome! Great improvement 👏 Feel free to merge one CI passes 👍
Think I'll make a new release once this is in, we have lots of good stuff and fixes that haven't been shipped yet 😄 |
I think we need to find a CI solution other than Travis 😅 |
Yeah, Travis build is still in progress! 😳 |
Something must have gone wrong with the Travis build. |
@mattiashagstrand I'm just gonna cut Travis off for now, we still have BuddyBuild until we can find a better solution, cuz this ain't working 😅 |
This PR fixes a bug in Block that caused strange rendering on macOS because of the flipped y axis.