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.

Sign up for GitHub

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

Jump to bottom

GLKit workaround #trivial #54

Merged
merged 4 commits into from
Apr 22, 2017
Merged

GLKit workaround #trivial #54

merged 4 commits into from
Apr 22, 2017

Conversation

stephenkopylov
Copy link
Contributor

canClearContentsOfLayer now turned off for views with CAEAGLLayer

more:
#53

@stephenkopylov
GLKit workaround:
617bc7d 
canClearContentsOfLayer now turned off for views with CAEAGLLayer
@CLAassistant
Copy link

CLAassistant commented Apr 21, 2017 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@stephenkopylov stephenkopylov mentioned this pull request Apr 21, 2017
Closed
nguyenhuy
nguyenhuy approved these changes Apr 21, 2017
View reviewed changes
Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

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

Nits aside, LGTM although I'd prefer another approval from a core maintainer before merging.

Source/ASDisplayNode.mm Outdated

// CAEAGLLayer
if([view.layer.class isSubclassOfClass:[CAEAGLLayer class]]){
_flags.canClearContentsOfLayer = NO;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Indentation is off

Source/ASDisplayNode.mm Outdated
@@ -588,6 +588,11 @@ - (UIView *)_locked_viewToLoad
|| [_viewClass isSubclassOfClass:[UIVisualEffectView class]]) {
self.opaque = NO;
}

// CAEAGLLayer
if([view.layer.class isSubclassOfClass:[CAEAGLLayer class]]){
Copy link
Member

Choose a reason for hiding this comment

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

Nit: [view.layer class] for consistency.

@stephenkopylov
Copy link
Contributor Author

Fxd

maicki
maicki approved these changes Apr 21, 2017
View reviewed changes
Copy link
Contributor

@maicki maicki left a comment

Choose a reason for hiding this comment

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

We may should start now thinking about a better structure around specific classes that need special handling as there are quite a couple different cases now. But this looks good to me.

@nguyenhuy
Copy link
Member

@maicki Agreed!

@nguyenhuy
Copy link
Member

nguyenhuy commented Apr 21, 2017 •
edited
Loading

@stephenkopylov Would you mind signing the CLA?
@maicki Can you kick off CI build?

@stephenkopylov
Copy link
Contributor Author

@nguyenhuy
I signed it, but notification is still here...

2017-04-21 22 34 19

@nguyenhuy
Copy link
Member

nguyenhuy commented Apr 21, 2017 •
edited
Loading

@stephenkopylov Could you try to add the email to your account?

Stepan Kopylov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@stephenkopylov
Copy link
Contributor Author

@nguyenhuy hooray! that was because I pushed PR from office, where I use different email.

@stephenkopylov
Copy link
Contributor Author

Sry guys, I missed one square bracket. Fxd

@ghost
Copy link

ghost commented Apr 22, 2017

2 Errors
🚫 Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.
🚫 Please ensure new source files begin with:

//
//  ASDisplayNode.mm
//  Texture
//
//  Copyright (c) 2014-present, Facebook, Inc.  All rights reserved.
//  This source code is licensed under the BSD-style license found in the
//  LICENSE file in the root directory of this source tree. An additional grant
//  of patent rights can be found in the PATENTS file in the same directory.
//  Modifications to this file made after 4/13/2017 are: Copyright (c) 2017-present,
//  Pinterest, Inc.  Licensed under the Apache License, Version 2.0 (the "License");
//  you may not use this file except in compliance with the License.
//  You may obtain a copy of the License at
//  
//      http://www.apache.org/licenses/LICENSE-2.0
//
//  Unless required by applicable law or agreed to in writing, software
//  distributed under the License is distributed on an "AS IS" BASIS,
//  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
//  See the License for the specific language governing permissions and
//  limitations under the License.
//

Generated by 🚫 Danger

@stephenkopylov stephenkopylov changed the title GLKit workaround: GLKit workaround #trivial Apr 22, 2017
nguyenhuy
nguyenhuy approved these changes Apr 22, 2017
View reviewed changes
Source/ASDisplayNode.mm
@@ -1,11 +1,23 @@
//
// ASDisplayNode.mm
// AsyncDisplayKit
// Texture
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

@nguyenhuy
Copy link
Member

nguyenhuy commented Apr 22, 2017 •
edited
Loading

@stephenkopylov Thanks for being patient and working through all of the hurdles. Hope to see more PRs from you!

@nguyenhuy nguyenhuy merged commit 1f69274 into TextureGroup:master Apr 22, 2017
@CoderXL CoderXL mentioned this pull request Sep 28, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maicki maicki maicki approved these changes

@nguyenhuy nguyenhuy nguyenhuy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stephenkopylov @CLAassistant @nguyenhuy @maicki