-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ASTextNode] Implement an example comparing ASTextNode 1 & 2 behavior. #570
[ASTextNode] Implement an example comparing ASTextNode 1 & 2 behavior. #570
Conversation
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
Source/ASTextNode.mm
Outdated
@@ -396,7 +396,7 @@ - (CGSize)calculateSizeThatFits:(CGSize)constrainedSize | |||
// Add the constrained size back textContainerInset | |||
size.width += (_textContainerInset.left + _textContainerInset.right); | |||
size.height += (_textContainerInset.top + _textContainerInset.bottom); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wsdwsd0829 you can delete these whitespace changes, and also revert the diffs to Kittens / CatDealsCollectionView / ASDKgram.
Will catch up with you offline to talk about potentially putting the new view controller in examples_extra/TextStressTest; this would reduce some of the other boilerplate and might help us centralize our reproduction cases for text drawing issues.
|
||
ASDisplayNode* l1Container = [ASDisplayNode yogaVerticalStack]; | ||
|
||
//TODO(fix ASTextNode2) uncomment next two line will show the bug of TextNode2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wsdwsd0829 Let's add a #define USE_ASTEXTNODE_2 0 at the top of the file, and we can switch on that in both the definitions of the instance variables / allocation of them in init. This will make it easier to switch between ASTextNode 1 and 2 to compare behavior.
We could also have a flag for enabling or disabling the Center + Start code. Or, can we just leave this on if it's necessary to show the rendering difference?
If I recall correctly, you were still seeing a bug even with the center + start lines commented out. Is that right, or does the behavior of ASTextNode 1 & 2 match without those lines? Do either of them match UILabel?
@wsdwsd0829 Thanks for working on this! Could you add some screenshots & description of the behavior you're seeing, with and without the cache workaround? This will help us get advice from Texture folks. |
Add flags to control different TextNode used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Thanks for the contribution!
} | ||
|
||
- (NSInteger)collectionNode:(ASCollectionNode *)collectionNode numberOfItemsInSection:(NSInteger)section { | ||
return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's totally fine to use a collection node even if there is only 1 item, it seems a bit overkill to me. A simple display node or even a scroll node should also do the trick. Not a blocker though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, Just incase I may add more examples to the collection later.
@wsdwsd0829 Please update the license of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@nguyenhuy Not to be picky for examples, but you think in terms of code styling (brace at new line after method etc.), should this be the same style as other examples or Texture in general?
Oh, I didn’t pay much attention to the styling. Yes, let’s have the same style across the repo. |
Thanks for the review guys! Yes, definitely we should match the repo style
before landing.
…On Fri, Oct 13, 2017 at 8:57 AM Huy Nguyen ***@***.***> wrote:
Oh, I didn’t pay much attention to the styling (and I’ve been reading
Kotlin/Java in thre last few days). Anyways, yes, let’s have the same style
across the repo.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#570 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAigAzZhdSYcD78-rwF46tNgPTIeUnCRks5sr4htgaJpZM4PYgZX>
.
|
🚫 CI failed with log |
Style fixed, thanks for the suggestions. |
🚫 CI failed with log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
TextureGroup#570) * fix SIMULATE_WEB_RESPONSE not imported TextureGroup#449 * add constraint to using catching for layout * Add TextNode example to test Yoga Layout * update Yoga version * add debugging log * fix lisence * clean up * clean up * fix lisence warning * add shared scheme * change sdk version * revert some metadata * Merge FlexLayoutExample to TextStressText. Add flags to control different TextNode used. * clean up * fix lisence and syntax * clean up * remove xcworkspacedata * Tiny coding style changes * Another tiny change related to code style
This add a new example project.
The purpose is to test Flex layout and its related bugs.
The structure is CollectionView with one row to show a layout example. We can add more rows as needed.
See TextCellNode example is to show a bug I am trying to resolve at #553