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

Set default tuning params #1158

Merged
merged 38 commits into from
Oct 10, 2018

Conversation

wsdwsd0829
Copy link
Contributor

It's valid that user may set tuning params to zero before view is loaded and it's better to provide default values.

It will fail to build if photo library is disabled cause the test is
depending on it.
Copy link
Member

@appleguy appleguy left a comment

Choose a reason for hiding this comment

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

Solid implementation approach, thanks for fixing this!

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.

I agree that it makes sense to have default turning params. Let's work on exposing the right API.

@@ -20,6 +22,8 @@ AS_EXTERN CGRect CGRectExpandToRangeWithScrollableDirections(CGRect rect, ASRang

@interface ASAbstractLayoutController : NSObject <ASLayoutController>

+ (std::vector<std::vector<ASRangeTuningParameters>>)defaultTuningParameters;
Copy link
Member

@nguyenhuy nguyenhuy Oct 3, 2018

Choose a reason for hiding this comment

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

Since this is a public header, I'm a bit worried about exposing Obj-C++ here since it's going to cause a cascade effect on every header file that imports it and every impl file that imports any of these headers. We're already seeing the effect in this PR.

How about we expose a Obj-C method here that converts the Obj-C data structure into a vector? Then maybe expose this Obj-C++ version in a framework private header, in case we want to mitigate any perf concern regarding the conversion.

How about exposing this Obj-C++ method in a framework private header?

@ghost
Copy link

ghost commented Oct 3, 2018

🚫 CI failed with log

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.

Look great to me!

@ghost
Copy link

ghost commented Oct 6, 2018

🚫 CI failed with log

@ghost
Copy link

ghost commented Oct 8, 2018

🚫 CI failed with log

@ghost
Copy link

ghost commented Oct 8, 2018

🚫 CI failed with log

@Adlai-Holler Adlai-Holler merged commit affddb0 into TextureGroup:master Oct 10, 2018
@ghost
Copy link

ghost commented Oct 22, 2018

🚫 CI failed with log

@ghost
Copy link

ghost commented Oct 22, 2018

🚫 CI failed with log

mikezucc pushed a commit to mikezucc/Texture that referenced this pull request Nov 7, 2018
* fix SIMULATE_WEB_RESPONSE not imported TextureGroup#449

* Fix to make rangeMode update in right time

* remove uncessary assert

* Fix collection cell editing bug for iOS 9 & 10

* Revert "Fix collection cell editing bug for iOS 9 & 10"

This reverts commit 06e18a1.

* Only test when photo library is enabled.

It will fail to build if photo library is disabled cause the test is
depending on it.

* Add ChangeLog.

* set default tuning parameters for collection/table node

* add change log

* Move to framework private.

* Apply to tableNode

* trigger ci

* fix directory

* fix file link
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.

5 participants