DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

Fixes #606 #610

Closed
kovpas wants to merge 4 commits from fix-606 into master
pull from: fix-606
merge into: DaddyFrosty:master
Conversation 34 Commits 4 Files Changed 2 +120 -242
kovpas commented 2017-07-28 02:44:54 -07:00 (Migrated from github.com)
Copy Link

Fixes #606.

If there are no subviews in UIView, yoga assumes that sizeThatFits: returns CGSizeZero. However, according to the documentation, UIView returns current size if there are no subviews.

This diff adds a check - if there are no subviews, sizeThatFits: doesn't get called, and CGSizeZero is returned.

Fixes #606. If there are no subviews in `UIView`, yoga assumes that `sizeThatFits:` returns `CGSizeZero`. However, according to [the documentation](https://developer.apple.com/documentation/uikit/uiview/1622625-sizethatfits), `UIView` returns current size if there are no subviews. This diff adds a check - if there are no subviews, `sizeThatFits:` doesn't get called, and CGSizeZero is returned.
👍 3
d16r (Migrated from github.com) requested changes 2017-07-28 10:13:28 -07:00
d16r (Migrated from github.com) left a comment
Copy Link

Looks good! Would you mind adding a unit test before we ship it?

Thanks!

Looks good! Would you mind adding a unit test before we ship it? Thanks!
kovpas commented 2017-07-28 11:23:08 -07:00 (Migrated from github.com)
Copy Link

@dshahidehpour sure! One question: I'm trying to run tests with

buck test --verbose 0 //YogaKit:YogaKitTests --config cxx.default_platform=iphonesimulator10.3

but I get an error:

BUILD FAILED: //YogaKit:YogaKitTests: Apple test requires an Apple platform, found 'default'

How do I fix that? Or is there an alternative way of running tests?

@dshahidehpour sure! One question: I'm trying to run tests with ``` buck test --verbose 0 //YogaKit:YogaKitTests --config cxx.default_platform=iphonesimulator10.3 ``` but I get an error: ``` BUILD FAILED: //YogaKit:YogaKitTests: Apple test requires an Apple platform, found 'default' ``` How do I fix that? Or is there an alternative way of running tests?
d16r commented 2017-07-28 18:21:29 -07:00 (Migrated from github.com)
Copy Link

Yeah, you can open the Xcode workspace and run them that way

Yeah, you can open the Xcode workspace and run them that way
kovpas commented 2017-07-31 07:48:29 -07:00 (Migrated from github.com)
Copy Link

Implemented tests. There's a problem on travis ci, but master is also failing.

Implemented tests. There's a problem on travis ci, but master is also failing.
kovpas commented 2017-08-15 06:18:14 -07:00 (Migrated from github.com)
Copy Link

@dshahidehpour gentle ping on this one. Could you please have a look?

@dshahidehpour gentle ping on this one. Could you please have a look?
d16r (Migrated from github.com) requested changes 2017-08-16 13:45:25 -07:00
YogaKit/Source/YGLayout.m
d16r (Migrated from github.com) commented 2017-08-16 13:45:20 -07:00
Copy Link

why do we have to check it's a UIView or not?

why do we have to check it's a `UIView` or not?
kovpas (Migrated from github.com) reviewed 2017-08-16 22:47:48 -07:00
YogaKit/Source/YGLayout.m
kovpas (Migrated from github.com) commented 2017-08-16 22:47:48 -07:00
Copy Link

Because the other classes (like UILabel) return correct size for sizeThatFits if there are no subviews

Because the other classes (like UILabel) return correct size for sizeThatFits if there are no subviews
d16r (Migrated from github.com) reviewed 2017-08-21 12:15:18 -07:00
YogaKit/Source/YGLayout.m
d16r (Migrated from github.com) commented 2017-08-21 12:15:18 -07:00
Copy Link

I wonder if there is a cleaner way to do this...I don't want us to check the class of a view every time yoga measures it.

I wonder if there is a cleaner way to do this...I don't want us to check the class of a view every time yoga measures it.
kovpas (Migrated from github.com) reviewed 2017-08-21 12:39:56 -07:00
YogaKit/Source/YGLayout.m
kovpas (Migrated from github.com) commented 2017-08-21 12:39:56 -07:00
Copy Link

Do you have ideas by any chance? The problem is that it seems that it's only UIView that results in this "odd" behavior - returns current size when it's empty.

Do you have ideas by any chance? The problem is that it seems that it's only UIView that results in this "odd" behavior - returns current size when it's empty.
kovpas (Migrated from github.com) reviewed 2017-08-22 02:18:26 -07:00
YogaKit/Source/YGLayout.m
kovpas (Migrated from github.com) commented 2017-08-22 02:18:26 -07:00
Copy Link

An option could be to create a flag isUIView in YGLayout and initialize it in initWithView:. What would you say?

An option could be to create a flag `isUIView` in `YGLayout` and initialize it in `initWithView:`. What would you say?
d16r (Migrated from github.com) reviewed 2017-08-22 08:05:04 -07:00
YogaKit/Source/YGLayout.m
d16r (Migrated from github.com) commented 2017-08-22 08:05:04 -07:00
Copy Link

I like it! Lets give it a shot.

I like it! Lets give it a shot.
kovpas commented 2017-08-28 07:45:45 -07:00 (Migrated from github.com)
Copy Link

@dshahidehpour gentle ping. Could you please have a look?

@dshahidehpour gentle ping. Could you please have a look?
kovpas commented 2017-09-06 01:51:46 -07:00 (Migrated from github.com)
Copy Link

Hey @dshahidehpour any news on this one?

Hey @dshahidehpour any news on this one?
👍 1
d16r commented 2017-09-07 13:34:49 -07:00 (Migrated from github.com)
Copy Link

Sorry @kovpas, I haven't had the bandwidth to merge this. Maybe @emilsjolander could help?

Sorry @kovpas, I haven't had the bandwidth to merge this. Maybe @emilsjolander could help?
d16r commented 2017-09-07 13:34:55 -07:00 (Migrated from github.com)
Copy Link

I'll try ASAP

I'll try ASAP
👍 1
kovpas commented 2017-09-27 03:04:25 -07:00 (Migrated from github.com)
Copy Link

Hey, @dshahidehpour! Just wondering if there's any progress on the review?

Hey, @dshahidehpour! Just wondering if there's any progress on the review?
d16r commented 2017-10-19 14:09:01 -07:00 (Migrated from github.com)
Copy Link

Hey @kovpas I'm really sorry for the delay. I've moved onto a different project at FB, so I'm finding a new POC who will be able to attend to your PRs in a much more timely fashion, stay-tuned!

Hey @kovpas I'm really sorry for the delay. I've moved onto a different project at FB, so I'm finding a new POC who will be able to attend to your PRs in a much more timely fashion, stay-tuned!
kovpas commented 2017-10-19 14:28:38 -07:00 (Migrated from github.com)
Copy Link

Thanks @dshahidehpour, and good luck with a new project! 👍

Thanks @dshahidehpour, and good luck with a new project! 👍
kovpas commented 2017-11-15 02:31:04 -08:00 (Migrated from github.com)
Copy Link

Hey @dshahidehpour any news on that? Could you please point me to a person, who could merge the pull request?

Hey @dshahidehpour any news on that? Could you please point me to a person, who could merge the pull request?
👍 2
matthargett (Migrated from github.com) approved these changes 2018-01-24 11:32:54 -08:00
kovpas commented 2018-01-24 13:40:43 -08:00 (Migrated from github.com)
Copy Link

Thanks for approving @matthargett! Could you please also merge the PR? I don’t have rights to do that.

Thanks for approving @matthargett! Could you please also merge the PR? I don’t have rights to do that.
priteshrnandgaonkar (Migrated from github.com) reviewed 2018-01-25 03:06:49 -08:00
YogaKit/Source/YGLayout.m
priteshrnandgaonkar (Migrated from github.com) commented 2018-01-25 03:06:49 -08:00
Copy Link

Here the view passed in the initialiser is of type UIView. Why do we need to have a bool with [view isMemberOfClass:[UIView class]] variable?

Here the `view` passed in the initialiser is of type `UIView`. Why do we need to have a bool with `[view isMemberOfClass:[UIView class]]` variable?
priteshrnandgaonkar (Migrated from github.com) reviewed 2018-01-25 03:07:39 -08:00
YogaKit/Source/YGLayout.m
@@ -304,10 +306,19 @@ static YGSize YGMeasureView(
const CGFloat constrainedHeight = (heightMode == YGMeasureModeUndefined) ? CGFLOAT_MAX: height;
priteshrnandgaonkar (Migrated from github.com) commented 2018-01-25 03:07:39 -08:00
Copy Link

Instead of pointing to the issue. Can you comment in the code, why this check is added.

Instead of pointing to the issue. Can you comment in the code, why this check is added.
priteshrnandgaonkar (Migrated from github.com) reviewed 2018-01-25 03:34:52 -08:00
YogaKit/Source/YGLayout.m
@@ -304,10 +306,19 @@ static YGSize YGMeasureView(
const CGFloat constrainedHeight = (heightMode == YGMeasureModeUndefined) ? CGFLOAT_MAX: height;
priteshrnandgaonkar (Migrated from github.com) commented 2018-01-25 03:34:52 -08:00
Copy Link

I think !view.yoga.isUIView this check is not required. Happy to hear about your inputs

I think `!view.yoga.isUIView` this check is not required. Happy to hear about your inputs
priteshrnandgaonkar (Migrated from github.com) requested changes 2018-01-25 03:36:18 -08:00
priteshrnandgaonkar (Migrated from github.com) left a comment
Copy Link

Thanks for the PR. I have written few review comments

Thanks for the PR. I have written few review comments
kovpas (Migrated from github.com) reviewed 2018-01-25 03:50:57 -08:00
YogaKit/Source/YGLayout.m
kovpas (Migrated from github.com) commented 2018-01-25 03:50:57 -08:00
Copy Link

Because it could also be a UILabel, UIButton and any other subclass of UIView. isMemberOfClass: makes sure that the view parameter is exactly UIView, not a subclass.

Because it could also be a `UILabel`, `UIButton` and any other subclass of `UIView`. `isMemberOfClass:` makes sure that the `view` parameter is exactly `UIView`, not a subclass.
kovpas (Migrated from github.com) reviewed 2018-01-25 03:54:54 -08:00
YogaKit/Source/YGLayout.m
@@ -304,10 +306,19 @@ static YGSize YGMeasureView(
const CGFloat constrainedHeight = (heightMode == YGMeasureModeUndefined) ? CGFLOAT_MAX: height;
kovpas (Migrated from github.com) commented 2018-01-25 03:54:45 -08:00
Copy Link

this condition practically says "we want to invoke sizeThatFits only in case if the measured view is not a UIView instance or it's a UIView, but has got children". The check is needed because we do want to invoke sizeThatFits for a UILabel (for example) that has got no subviews, but returns a correct value as a result of invocation of sizeThatFits

this condition practically says "we want to invoke `sizeThatFits` only in case if the measured view is not a `UIView` instance or it's a `UIView`, but has got children". The check is needed because we do want to invoke `sizeThatFits` for a UILabel (for example) that has got no subviews, but returns a correct value as a result of invocation of `sizeThatFits`
kovpas (Migrated from github.com) reviewed 2018-01-25 03:58:49 -08:00
YogaKit/Source/YGLayout.m
@@ -304,10 +306,19 @@ static YGSize YGMeasureView(
const CGFloat constrainedHeight = (heightMode == YGMeasureModeUndefined) ? CGFLOAT_MAX: height;
kovpas (Migrated from github.com) commented 2018-01-25 03:58:49 -08:00
Copy Link

Done.

Done.
priteshrnandgaonkar (Migrated from github.com) reviewed 2018-01-25 03:59:10 -08:00
YogaKit/Source/YGLayout.m
@@ -304,10 +306,19 @@ static YGSize YGMeasureView(
const CGFloat constrainedHeight = (heightMode == YGMeasureModeUndefined) ? CGFLOAT_MAX: height;
priteshrnandgaonkar (Migrated from github.com) commented 2018-01-25 03:59:10 -08:00
Copy Link

Aah Got it. Can you add a comment(in brief) explaining this before the condition?

Aah Got it. Can you add a comment(in brief) explaining this before the condition?
kovpas commented 2018-01-25 03:59:43 -08:00 (Migrated from github.com)
Copy Link

@priteshrnandgaonkar thanks for the comments. Addressed all of them.

@priteshrnandgaonkar thanks for the comments. Addressed all of them.
kovpas (Migrated from github.com) reviewed 2018-01-25 04:02:34 -08:00
YogaKit/Source/YGLayout.m
@@ -304,10 +306,19 @@ static YGSize YGMeasureView(
const CGFloat constrainedHeight = (heightMode == YGMeasureModeUndefined) ? CGFLOAT_MAX: height;
kovpas (Migrated from github.com) commented 2018-01-25 04:02:34 -08:00
Copy Link

Done

Done
facebook-github-bot (Migrated from github.com) reviewed 2018-01-25 04:03:17 -08:00
facebook-github-bot (Migrated from github.com) left a comment
Copy Link

@priteshrnandgaonkar has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@priteshrnandgaonkar has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D6807406).
pablosichert commented 2018-10-06 05:21:29 -07:00 (Migrated from github.com)
Copy Link

Is there still anything holding this off from being merged?

Is there still anything holding this off from being merged?
facebook-github-bot (Migrated from github.com) reviewed 2018-10-06 10:25:51 -07:00
facebook-github-bot (Migrated from github.com) left a comment
Copy Link

priteshrnandgaonkar has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

priteshrnandgaonkar has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.internmc.facebook.com/D6807406).

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
d16r
matthargett
priteshrnandgaonkar
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
CLA Signed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#610
No description provided.
Delete Branch "fix-606"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?