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

Blended layers optimization #573

Merged
merged 18 commits into from
Mar 21, 2019

Conversation

RobertPieta
Copy link
Contributor

📲 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

@justinswart
Copy link
Contributor

hey thanks for the suggestion @RobertPieta! I will get our CI to run tests on this and chat to the team about it!

@dusi
Copy link
Contributor

dusi commented Feb 4, 2019

Thanks @RobertPieta ! We'll try to keep an eye on this more rigorously 🔬

@justinswart
Copy link
Contributor

@RobertPieta I've enabled tests to run on your fork so that we can work to get this merged.

@dusi
Copy link
Contributor

dusi commented Feb 22, 2019

@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:

  1. Select iPhone 8 as the destination device (our tests run against iPhone 8 devices only)
  2. Run the test suite (CMD+U) in order to see all the failing tests
  3. Re-record any failing tests

This could be done by going to a failing test / or a failing test class and turning the recordMode flag ON.

So for example recording new snapshots for the BackerDashboardProjectsViewControllerTests failing test would be as simple as running all of the BackerDashboardProjectsViewControllerTests tests with recordMode = true

// BackerDashboardProjectsViewControllerTests.swift
override func setUp() {
  super.setUp()
  AppEnvironment.pushEnvironment(mainBundle: Bundle.framework)
  UIView.setAnimationsEnabled(false)

  // Run tests once in record mode ... then remove and next time you run the tests they should pass
  self.recordMode = true
}

(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 rebase/merge master in order to resolve possible merge conflicts. We have shipped some major changes after you've opened this PR.

Let me know if you have any questions.

@dusi
Copy link
Contributor

dusi commented Feb 22, 2019

Also (just for our future reference) I'm posting this comment to reference the changes done with this PR (for better illustration)

Before After
screen shot 2019-02-20 at 4 05 11 pm screen shot 2019-02-20 at 4 05 10 pm
screen shot 2019-02-20 at 4 06 12 pm screen shot 2019-02-20 at 4 06 11 pm
screen shot 2019-02-20 at 4 06 18 pm screen shot 2019-02-20 at 4 06 17 pm
screen shot 2019-02-20 at 4 06 41 pm screen shot 2019-02-20 at 4 06 39 pm
screen shot 2019-02-20 at 4 08 03 pm screen shot 2019-02-20 at 4 08 02 pm

@justinswart
Copy link
Contributor

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, self.recordMode = true in setUp() in TestCase.swift which is the superclass for all of our snapshot tests.

https://github.com/kickstarter/ios-oss/blob/master/Library/TestHelpers/TestCase.swift#L29

@dusi dusi self-assigned this Mar 19, 2019
Copy link
Contributor

@dusi dusi left a 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)

@dusi dusi merged commit fe5ce04 into kickstarter:master Mar 21, 2019
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.

3 participants