-
-
Notifications
You must be signed in to change notification settings - Fork 586
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
Support flexWrap
in Box
#479
Conversation
#478 must be merged first |
@sindresorhus @SimenB adjusted :) |
any updates? I need this on NPM :) |
I'm not a maintainer, sorry 🙂 |
It will need to be reviewed by @vadimdemedes
|
</Box> | ||
); | ||
|
||
t.is(output, 'BC\n'); |
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.
Is it expected that there's no A
here?
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.
not really, by default overflow is expected to be visible in flex-box layout. this can be seen with plain Yoga:
https://replit.com/@SubhiAl/MonumentalNegligibleTrust#index.js
I needed to add the test to show the difference. The behaviour was there even before my changes.
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.
It turns out that this is behaviour is caused by applying flexShrink=1
on each text node:
https://github.com/vadimdemedes/ink/blob/master/src/components/Text.tsx#L114
Changing flexShrink to 0 makes 'A' visible in the output
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.
I was also able to reproduce the behaviour in plain Yoga:
https://replit.com/@SubhiAl/AwfulThreadbareMicroinstruction#index.js
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.
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.
👋
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.
@vadimdemedes sorry to spam you, but can this be merged?
@vadimdemedes i see that you reopened the PR. what is the next step to get this baked in? |
@vadimdemedes @jodevsa would love to get this in! Could we reopen the PR and work towards merging it? |
@matteodepalo same here :( it seems like that this repo is no longer maintained anymore. I tried hard to push for someone to merge my change. |
@jodevsa that's a shame. I think it's be worth keeping the PR open if you think it's complete and wait for a maintainer's comment. Is there any specific reason you closed it? |
@matteodepalo not really, I've closed it to be able to track the things I'm working on as I usually ignore my notifications because they are distracting and stressful and look on my open PRs every day. I hope someday someone would improve GitHub notifications. I've reopened the PR. @vadimdemedes it would be really nice if we can get this in someday :) |
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.
Thank you and sorry for taking so long 💛
adding support for flexWrap
motivation:
Building a flashcard app
Also contributes to #464