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

[TIMOB-13294] new clipChildren property #2781

Closed
wants to merge 9 commits into from
Closed

[TIMOB-13294] new clipChildren property #2781

wants to merge 9 commits into from

Conversation

farfromrefug
Copy link
Contributor

@farfromrefug
clipToBounds was set on every children addition. Instead do it once a…
02d3251 
…t init. And then it allows to set it through property

Conflicts:
	iphone/Classes/TiUIView.m
@farfromrefug
clipChildren property for android
c96395d 
Conflicts:
	android/titanium/src/java/org/appcelerator/titanium/view/TiUIView.java
@mstepanov
Copy link
Contributor

Misses proper documentation for the new property.

mstepanov
mstepanov reviewed Aug 28, 2012
View reviewed changes
android/titanium/src/java/org/appcelerator/titanium/view/TiUIView.java
}
} else if (key.equals(TiC.PROPERTY_CLIP_CHILDREN)) {
if (nativeView instanceof TiCompositeLayout) {
boolean clip = TiConvert.toBoolean(proxy.getProperty(TiC.PROPERTY_CLIP_CHILDREN));
Copy link
Contributor

Choose a reason for hiding this comment

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

why newValue is ignored ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oups! my bad, will correct that right away

@mstepanov
Copy link
Contributor

Code reviewed. APPROVED

@farfromrefug
Copy link
Contributor Author

Thanks @mstepanov for taking the time to look at this. I think it can be a great addition

@mstepanov
Copy link
Contributor

Functional test PASSED on iOS. FAILED on Android.
The JIRA test case is not quite correct too.

@farfromrefug
Copy link
Contributor Author

I fixed the testcase, i was working with kitchensink :s sorry

Now you are right something is wrong with the android impl. I thought it would be pretty easy but it wasnt.
I am wondering if it could be due to this line:
https://github.com/appcelerator/titanium_mobile/blob/master/android/titanium/src/java/org/appcelerator/titanium/view/TiBorderWrapperView.java#L65

Also i have read on stackoverflow that i might have to set clipChildren for all parents!

Will look into it

@farfromrefug
removed clipChildren on android as it is not working. Don't really se…
9cdc4ce 
…e how to achieve it.

Conflicts:
	android/titanium/src/java/org/appcelerator/titanium/view/TiUIView.java
@farfromrefug
Copy link
Contributor Author

Ok so sadly i had to remove the android implementation. I cant seem to find a way to do that.
Actually the only i managed to do something close to this on android was with relativeLayout, clipPadding and marginLeft...
And i cant port that to TiCompositeLayout.

@farfromrefug
Copy link
Contributor Author

i just found out about 2 lines that could be a problem on ios
https://github.com/appcelerator/titanium_mobile/blob/master/iphone/Classes/TiUIView.m#L501
I dont really see the point there

https://github.com/appcelerator/titanium_mobile/blob/master/iphone/Classes/TiUIView.m#L533
there it is not necessary as clipToBounds only affect sub layers, not content layer.

will comment them

@mstepanov
Copy link
Contributor

We definitely need clipping in both these cases.
Technically we should have do it with self.layer.maskToBounds in both. Try that to see if it breaks your feature.

@farfromrefug
Copy link
Contributor Author

thanks @mstepanov will try this

@mstepanov
Copy link
Contributor

  1. clipChildren property documentation is still missing.
  2. I see absence of Android implementation as a serious issue here.

Input from @ayeung @billdawson @DizzyMonkey needed

@farfromrefug
Copy link
Contributor Author

@mstepanov sorry i didnt see the comment about documentation. Will add it.
As for android implementation, as i explained it is pretty hard to do, which means i actually dont see how to do it within Ti architecture. And i also think it s an issue.
Still this property is much needed, and having it on ios is already a good thing.

@farfromrefug
Copy link
Contributor Author

a note on this one
In ios we have a few self.clipsToBounds floating around.
Especially there is one in TiUIView that i dont really get.
It s related to another pull request that i just made.
It breaks IMO the behavior of backgroundPadding of label.

@ghost ghost assigned mstepanov Oct 3, 2012
@farfromrefug
Copy link
Contributor Author

any news on this? I added the documentation

@ingo
Copy link
Contributor

ingo commented Feb 21, 2014

Apologies for the late update. We are tentatively considering this for 3.3.0.

@vishalduggal vishalduggal mentioned this pull request Feb 23, 2014
Merged
@vishalduggal
Copy link
Contributor

@farfromrefug
Thanks for the PR. We have incorporated this functionality in PR #5367. (clipMode property on Ti.UI.View for iOS). Closing out this PR.

@farfromrefug
Copy link
Contributor Author

I have seen your PR, very nice @vishalduggal
Thanks for letting me know

@farfromrefug farfromrefug deleted the TC-1186 branch February 24, 2014 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@farfromrefug @mstepanov @ingo @vishalduggal