-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Blended layers optimization #573
Blended layers optimization #573
Conversation
hey thanks for the suggestion @RobertPieta! I will get our CI to run tests on this and chat to the team about it! |
Thanks @RobertPieta ! We'll try to keep an eye on this more rigorously 🔬 |
@RobertPieta I've enabled tests to run on your fork so that we can work to get this merged. |
@RobertPieta as Justin mentioned, he has enabled CircleCI to run the tests against your branch (we've previously only been able to do that locally for public PRs). In order for us to being able to merge, would it be possible for you to update all the failing Snapshot tests? You should be able to resolve this by following these steps:
This could be done by going to a failing test / or a failing test class and turning the So for example recording new snapshots for the
(this will unfortunately have to be manually done for all the failing test classes. These new snapshots will have to be committed and pushed to the repo (and be part of this PR) in order for CircleCI to pass. Setting background colours explicitly isn't visually different to a naked eye, but Snapshots tests treat this fact as a change so the snapshot has to be re-recorded. ps: I believe you might also need to Let me know if you have any questions. |
Thanks @dusi for the detailed explanation. @RobertPieta the simplest way to re-record all of the snapshots is to put the line that @dusi mentioned, https://github.com/kickstarter/ios-oss/blob/master/Library/TestHelpers/TestCase.swift#L29 |
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 again @RobertPieta .
I've double checked that this PR doesn't break any visual functionality (more specifically the following):
✅ Classic Invert works
✅ Smart Invert works
✅ UI remains identical to human eye
🚢 🚢 🚢
🚢 🚢 🚢
🚢 🚢 🚢
Tested on iPhone X (iOS 12.1.4)
📲 What
Added explicit background color to labels and other views that are rendered as blended layers.
🤔 Why
Labels and some other views by default have a transparent background, requiring iOS to render a blended layer. Reducing blended layers increases performance by reducing the work iOS needs to do when drawing and can have a large impact on scrolling smoothness and frame rate.
🛠 How
Added explicit background color to labels and some other views using the existing configuration conventions in the project. When applicable, the background color was updated in an aggregate styles variable or assigned from a styles object.
✅ Acceptance criteria
[ ] Unit and screenshot comparison tests pass