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

Feature auto margin #357

Closed
woehrl01 wants to merge 2 commits from feature-auto-margin into master
pull from: feature-auto-margin
merge into: DaddyFrosty:master
Conversation 35 Commits 2 Files Changed 28 +3353 -58
woehrl01 commented 2017-01-26 16:06:31 -08:00 (Migrated from github.com)
Copy Link

Even so I know there are some opinions against margin: 0 auto it's still part of the spec: https://www.w3.org/TR/css-flexbox-1/#auto-margins and pretty usefull if you have to position via justify-content.

This PR adds an implementation for that.

It adds an additonal YGUnitAuto and margins got YGNodeStyleSetMarginAuto functions as well.

Even so I know there are some opinions against ```margin: 0 auto``` it's still part of the spec: https://www.w3.org/TR/css-flexbox-1/#auto-margins and pretty usefull if you have to position via ```justify-content```. This PR adds an implementation for that. It adds an additonal ```YGUnitAuto``` and margins got ```YGNodeStyleSetMarginAuto``` functions as well.
emilsjolander commented 2017-01-26 16:44:23 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 We don't aim to implement the full css spec. Could you add some examples of layouts which are made significantly easier / possible with margin auto support compared to implementing those layouts today?

@woehrl01 We don't aim to implement the full css spec. Could you add some examples of layouts which are made significantly easier / possible with margin auto support compared to implementing those layouts today?
woehrl01 commented 2017-01-27 00:01:39 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander I didn't came across any layout yet, which wasn't able to implement with yoga today. But I had to add spacer views. I want to get rid of spacer views as much as I can and the last "thing" can be only solved with auto margins.

I'm totally aware that there is no need for the full css spec, but as auto margins are explicitly mentioned and described in flex-box I would still account it to the flex-box model layouting logic.

@emilsjolander I didn't came across any layout yet, which wasn't able to implement with yoga today. But I had to add spacer views. I want to get rid of spacer views as much as I can and the last "thing" can be only solved with auto margins. I'm totally aware that there is no need for the full css spec, but as auto margins are explicitly mentioned and described in flex-box I would still account it to the flex-box model layouting logic.
emilsjolander commented 2017-01-27 09:35:21 -08:00 (Migrated from github.com)
Copy Link

I would count needing spacer views as a limitation. Could you give some examples of where margin auto would remove the need for spacer views?

I would count needing spacer views as a limitation. Could you give some examples of where margin auto would remove the need for spacer views?
woehrl01 commented 2017-01-27 11:12:34 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander Sure, see this simple JSFiddle.

It's pretty much any layout were you want to have elements on one side and some additonal on the other side e.g for menus.

I also use this e.g. for centered notification messages where I have additional text at the bottom (can be boiled down to the layout above but in column direction)

@emilsjolander Sure, see this simple [JSFiddle](https://jsfiddle.net/uojyuspt/). It's pretty much any layout were you want to have elements on one side and some additonal on the other side e.g for menus. I also use this e.g. for centered notification messages where I have additional text at the bottom (can be boiled down to the layout above but in column direction)
emilsjolander commented 2017-01-27 11:18:55 -08:00 (Migrated from github.com)
Copy Link

Thanks, that helps!

Thanks, that helps!
emilsjolander (Migrated from github.com) reviewed 2017-01-27 11:19:30 -08:00
gentest/fixtures/YGMarginTest.html
emilsjolander (Migrated from github.com) commented 2017-01-27 11:19:30 -08:00
Copy Link

This should be a fix in a seperate pull request.

This should be a fix in a seperate pull request.
emilsjolander (Migrated from github.com) reviewed 2017-01-27 11:20:54 -08:00
gentest/gentest-cpp.js
@@ -13,8 +13,10 @@ function toValueCpp(value) {
}
function toFunctionName(value) {
emilsjolander (Migrated from github.com) commented 2017-01-27 11:19:45 -08:00
Copy Link

space before else

space before `else`
gentest/gentest-cpp.js
emilsjolander (Migrated from github.com) commented 2017-01-27 11:20:10 -08:00
Copy Link

space before ( and after )

space before `(` and after `)`
emilsjolander (Migrated from github.com) commented 2017-01-27 11:20:18 -08:00
Copy Link

space before and after else

space before and after `else`
emilsjolander commented 2017-01-29 11:38:47 -08:00 (Migrated from github.com)
Copy Link

We currently use YGUndefined to mean auto for width, height, and flex-basis. I'm concerned with this being confusing after this PR introduced an explicit auto value for margin. If we introduce a specific auto value then I think this should be respected for other properties as well.

We currently use `YGUndefined` to mean `auto` for `width`, `height`, and `flex-basis`. I'm concerned with this being confusing after this PR introduced an explicit `auto` value for margin. If we introduce a specific `auto` value then I think this should be respected for other properties as well.
woehrl01 commented 2017-01-29 12:02:02 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander I thought about this too, should I add/change those? I would handle undefined the same like auto would on those values, so this wouldn't brake existing users.

@emilsjolander I thought about this too, should I add/change those? I would handle undefined the same like auto would on those values, so this wouldn't brake existing users.
emilsjolander commented 2017-01-29 12:09:28 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 I'm not sure about the best way to handle it, feel free to experiment. But yes, whatever we do we should not break existing usages.

@woehrl01 I'm not sure about the best way to handle it, feel free to experiment. But yes, whatever we do we should not break existing usages.
👍 1
woehrl01 commented 2017-01-29 13:12:31 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander I changed width, height and flex-basis to have auto as default value. undefined on the setter is silently converted to auto.

@emilsjolander I changed ```width```, ```height``` and ```flex-basis``` to have ```auto``` as default value. undefined on the setter is silently converted to ```auto```.
emilsjolander commented 2017-01-29 15:19:16 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 looks pretty good. I'll get back to you later this week regarding this.

@woehrl01 looks pretty good. I'll get back to you later this week regarding this.
emilsjolander (Migrated from github.com) requested changes 2017-02-01 01:51:25 -08:00
emilsjolander (Migrated from github.com) left a comment
Copy Link

I'm missing tests for auto margins with align stretch. The code seems to show that auto margins should override alignItems/alignself = stretch. We should test this.

For all your tests please

  1. end each style with a ;, even the last one.
  2. add a space between ';' and the next style in each style decleration

Also could you add language bindings for the new auto setters?

I'm missing tests for auto margins with align stretch. The code seems to show that auto margins should override alignItems/alignself = stretch. We should test this. For all your tests please 1. end each style with a `;`, even the last one. 2. add a space between ';' and the next style in each style decleration Also could you add language bindings for the new auto setters?
gentest/fixtures/YGMarginTest.html
@@ -39,3 +39,70 @@
<div style="margin-bottom: 10px; flex-grow: 1;"></div>
emilsjolander (Migrated from github.com) commented 2017-02-01 01:45:40 -08:00
Copy Link

add a similar test for row direction. Also please change name to something like margin_auto_multiple_children for a _row and _column suffix.

add a similar test for row direction. Also please change name to something like `margin_auto_multiple_children` for a `_row` and `_column` suffix.
woehrl01 commented 2017-02-01 10:00:01 -08:00 (Migrated from github.com)
Copy Link

I added most of the language bindings except objc and javascript. As I don't know objc good enough and for javascript I think it would be good to extend entry-common to also support "auto" as an additional short cut (the tests are generated for this) but I'm not familiar enough with this change.

Please have a look if I have missed something, as those language changes aren't tested. But most of the repetitive work should be done ;) which should get you started pretty quick.

I added most of the language bindings except objc and javascript. As I don't know objc good enough and for javascript I think it would be good to extend ```entry-common``` to also support ```"auto"``` as an additional short cut (the tests are generated for this) but I'm not familiar enough with this change. Please have a look if I have missed something, as those language changes aren't tested. But most of the repetitive work should be done ;) which should get you started pretty quick.
emilsjolander commented 2017-02-02 05:16:51 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 that's fine! I'll fix up javascript and objc when importing 👍 Thanks for this awesome addition 💯

@woehrl01 that's fine! I'll fix up javascript and objc when importing 👍 Thanks for this awesome addition 💯
facebook-github-bot commented 2017-02-02 05:27:07 -08:00 (Migrated from github.com)
Copy Link

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

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D4501142).
emilsjolander commented 2017-02-02 11:27:40 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 could you rebase this? Github says there are no conflicts but i'm having some issues importing it.

@woehrl01 could you rebase this? Github says there are no conflicts but i'm having some issues importing it.
woehrl01 commented 2017-02-02 11:42:41 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander merged the master for this and the other PR you didn't imported yet.

@emilsjolander merged the master for this and the other PR you didn't imported yet.
facebook-github-bot commented 2017-02-02 11:46:34 -08:00 (Migrated from github.com)
Copy Link

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

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D4501142).
emilsjolander commented 2017-02-02 11:50:51 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 Could you rebase this on top of master instead of merging master into this branch?

@woehrl01 Could you rebase this on top of master instead of merging master into this branch?
woehrl01 commented 2017-02-02 11:53:16 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander I'll try my best, I'm more of a mercurial than a git user. 😊

@emilsjolander I'll try my best, I'm more of a mercurial than a git user. 😊
emilsjolander commented 2017-02-02 11:54:19 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 lol me to. Another option is to squash the commit and the push it, that should work as well when I import.

@woehrl01 lol me to. Another option is to squash the commit and the push it, that should work as well when I import.
😆 1
facebook-github-bot commented 2017-02-02 12:02:12 -08:00 (Migrated from github.com)
Copy Link

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

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D4501142).
woehrl01 commented 2017-02-02 12:07:57 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander let me know if it works, otherwise I'll squash them all. 💪

@emilsjolander let me know if it works, otherwise I'll squash them all. 💪
emilsjolander commented 2017-02-02 12:09:55 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 it didn't work :/ can you try squashing?

@woehrl01 it didn't work :/ can you try squashing?
woehrl01 commented 2017-02-02 12:28:56 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander try this, please 🤘

@emilsjolander try this, please 🤘
facebook-github-bot commented 2017-02-02 12:31:07 -08:00 (Migrated from github.com)
Copy Link

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

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D4501142).
emilsjolander commented 2017-02-02 12:36:07 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 that worked! Thanks! sorry for the hassle.

@woehrl01 that worked! Thanks! sorry for the hassle.
facebook-github-bot commented 2017-02-02 12:36:17 -08:00 (Migrated from github.com)
Copy Link

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

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D4501142).
woehrl01 commented 2017-02-02 12:39:37 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander 👏 awesome! No worries, thanks to this I learned new things about git today 👍

@emilsjolander 👏 awesome! No worries, thanks to this I learned new things about git today 👍
emilsjolander commented 2017-02-08 07:37:35 -08:00 (Migrated from github.com)
Copy Link

I'm still working on this. It breaks some tests internally and I have a couple other things that are higher priority. hang tight 👍

I'm still working on this. It breaks some tests internally and I have a couple other things that are higher priority. hang tight 👍
woehrl01 commented 2017-02-08 07:48:26 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander If there is anything were I can help you out. Let me know it! No rush 👍

@emilsjolander If there is anything were I can help you out. Let me know it! No rush 👍
emilsjolander commented 2017-02-08 07:56:04 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 will do!

@woehrl01 will do!
woehrl01 commented 2017-02-12 02:49:26 -08:00 (Migrated from github.com)
Copy Link

@emilsjolander I did a rebase and fixed the auto margin as some of the behaviour this implicitly used has changed, now its explicit.

@emilsjolander I did a rebase and fixed the auto margin as some of the behaviour this implicitly used has changed, now its explicit.
facebook-github-bot commented 2017-02-13 00:26:13 -08:00 (Migrated from github.com)
Copy Link

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

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

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
emilsjolander
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#357
No description provided.
Delete Branch "feature-auto-margin"

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?