Gap/row-gap/column/gap properties

[jacobp100]

Report

• I have searched existing issues and this is not a duplicate

In the drafts of box alignment level 3, there's the properties row-gap and column-gap that allow the user to specify the gaps between rows and columns.

It is a draft, so it might be premature to implement now. Do we want to support this now? (I can take a shot at it if I get time!)

[lucadegasperi]

I'd like to resurface this issue as the gap property is slowly getting browser support. https://caniuse.com/#search=gap I believe this would be a huge win for everyone and would make creating layouts a lot easier.

[lostpebble]

Yep, this really is one of the final missing pieces in most "flexbox" implementations. Having to balance out margins for inner elements, which sometimes change position etc., is really not a fun experience.

[ricokahler]

I think this should get revisited as well. Flex gap has landed in stable for both chrome and firefox: https://caniuse.com/flexbox-gap

[efoken]

Would be nice to have this as browser support grows.

[ricokahler]

Update: it landed in webkit too: https://bugs.webkit.org/show_bug.cgi?id=206767

[AndrewPrifer]

I suggest renaming the issue to "Support gap property" to reflect changes to the name. By the way Safari Technology Preview also supports gap now.

[lwyj123]

It's seems like the gap properties is supported in modern browsers. I think this issues should be review again.

[AndrewPrifer]

Gap just dropped in Safari too.

[Sm0keySa1m0n]

Any updates on this?

[intergalacticspacehighway]

Created a draft PR on React native repo which includes gap related changes. Feedbacks are welcome 😄 facebook/react-native#32766

[jacobp100]

It looks promising! You'd need to do the PR here. I did have a stab at it, and there's a few differences:-

You'll need to add tests - the cpp files (here) are generated from html (here). They're run through Chrome to get the layouts, so it's very automatic.

I didn't think about doing this in getLeadingMargin. If that works, it makes it so much simpler. Let me know how the tests go!

I structured the gap like borders:-

yoga/yoga/YGEnums.h

Lines 78 to 88 in 578d197

	YG_ENUM_SEQ_DECL(
	YGEdge,
	YGEdgeLeft,
	YGEdgeTop,
	YGEdgeRight,
	YGEdgeBottom,
	YGEdgeStart,
	YGEdgeEnd,
	YGEdgeHorizontal,
	YGEdgeVertical,
	YGEdgeAll)

You'd end up with a gap property that would be an array of row and column. The advantage of this was you'd be able to set gap in react-native, and have it apply to both the row and column

[jacobp100]

But this is really cool to see! Really glad to have someone else working on it because I hit a brick wall with it. Happy to help out too.

[intergalacticspacehighway]

yeah, basically thought of getting row/column gap first. Gap would be simpler on top of it, will be a good addition!
will do PR here. Need to see how to build and run Yoga first. I'll ask you If I run into any issues. Thank you!

(regarding using margins -> Just thought about it today 😅 , earlier I was trying a different approach which got complicated )

[jacobp100]

Yeah I found building the project is quite hard too. Now I've replaced my laptop with an M1 and it seems like a no-go atm 😔

[intergalacticspacehighway]

Just managed to build and run tests in m1 (using rosetta for now). This is what I did to install. (this needs to be done on rosetta terminal)

brew install --cask homebrew/cask-versions/adoptopenjdk8   
brew install buck
brew install chromedriver
brew install watchman
gem install watir v6.19.1

Post install, I needed to do the following change because I installed latest chromedriver.

When I ran buck test //:yoga all the tests passed.

Interestingly when I generate tests from gentest.rb, I see some values change in tests, and three tests start failing. Not sure why (could be because I am using new chromedriver and browser behaviour changed (sounds unlikely)). This is without any gap-related changes. Investigating it further.

[jacobp100]

Ah amazing! I’ll give it a shot

[jacobp100]

I also got layout changes running it without changes on my old Intel machine

[jacobp100]

@intergalacticspacehighway I'm stuck with openjdk

Error: openjdk@8: no bottle available!
You can try to install from source with:
  brew install --build-from-source openjdk@8
Please note building from source is unsupported. You will encounter build
failures with some formulae. If you experience any issues please create pull
requests instead of asking for help on Homebrew's GitHub, Twitter or any other
official channels.

If I run that command, it installs a load of packages successfully, but then fails here,

==> Installing openjdk@8
==> common/autoconf/autogen.sh
==> ./configure --with-boot-jdk-jvmargs=-Duser.home=/Users/jacob/Library/Caches/Homebrew/java_cache --with-boot-jdk=/private/tmp/openjdkA8-20211219-60748-3o4zoe/jdk8u312-ga/boot-jdk --with-debug-level=release --wit
==> make bootcycle-images CONF=release
Last 15 lines from /Users/jacob/Library/Logs/Homebrew/openjdk@8/03.make:
2021-12-19 10:14:35 +0000

make
bootcycle-images
CONF=release

No configurations found for /private/tmp/openjdkA8-20211219-60748-3o4zoe/jdk8u312-ga/! Please run configure to create a configuration.
Makefile:55: *** Cannot continue.  Stop.

READ THIS: https://docs.brew.sh/Troubleshooting

These open issues may also help:
openjdk@8: depend on awk https://github.com/Homebrew/homebrew-core/pull/89982
openjdk@11 11.0.13 https://github.com/Homebrew/homebrew-core/pull/87638
OpenJDK is somewhat broken on newer MacOS instances, console is flooded with errors when using JMeter, AdoptOpenJDK has no issues https://github.com/Homebrew/homebrew-core/issues/66953

[intergalacticspacehighway]

This is weird. Can you try this command? (also make sure it's rosetta terminal 😅 )

brew install --cask homebrew/cask-versions/adoptopenjdk8  

Also, found an inconsistency in one of the failing tests. YGMinMaxDimensionTest.html's min_height test
Try this codepen. It behaves differently on chrome and safari. The test assertion written is based on safari behaviour but since gentest runs on newer chrome, we're seeing the changes for this test. Not sure. Will keep digging.

[jacobp100]

I did run that - no issues, it successfully installed. But it didn’t fix brew install buck

[intergalacticspacehighway]

Okay. Can you try two more things.

1. Try running brew doctor and see if it mentions any issues
2. Install new homebrew in the rosetta terminal and then install buck

// to install buck
brew tap facebook/fb
brew install buck     

[jacobp100]

Ah you're right - I wasn't using Rosetta. Ok so now I get a different error,

jacob@Jacobs-MacBook-Pro ~ % brew tap facebook/fb
jacob@Jacobs-MacBook-Pro ~ % brew install buck
Error: Cannot install under Rosetta 2 in ARM default prefix (/opt/homebrew)!
To rerun under ARM use:
    arch -arm64 brew install ...
To install under x86_64, install Homebrew into /usr/local.

I installed brew on arm64. I think stuff has got into a bit of a mess, so I'm gonna uninstall and re-install brew. Did you install brew under Rosetta or native arm64? And do you have your install command?

[intergalacticspacehighway]

Sorry it's a bit mess. I should've taken proper notes. I think I followed this instruction

Also, created this issue. I think I'll ignore this issue for now and test with gap-related cases.

[jacobp100]

I've started my branch for gap over here - https://github.com/jacobp100/yoga/tree/gap

So far just in process the proper infrastructure so you can set gap, row-gap, and column-gap

[jacobp100]

Thanks for your help! Buck is now running. I'm getting the same as you for tests - but I'm sure that's just the tests haven't been run in a while. Will let you know how I get on with the gap stuff.

[intergalacticspacehighway]

Awesome. I forgot to mention you that I've implemented gap on the same react native draft PR. These are the changes for Yoga. Let me know what you think about this approach. Will be adding testcases.

[jacobp100]

I think there's an issue using owner -

  // returns the YGNodeRef that owns this YGNode. An owner is used to identify
  // the YogaTree that a YGNode belongs to. This method will return the parent
  // of the YGNode when a YGNode only belongs to one YogaTree or nullptr when
  // the YGNode is shared between two or more YogaTrees.
  YGNodeRef getOwner() const { return owner_; }

[intergalacticspacehighway]

I think it would be an issue when a YGNode is shared between two or more YogaTrees. Is this possible? Can a child have 2 parents at the same time!?

Using owner because gap is set on the parent node and we need a way to get it in the child node. Not sure if there's any alternate way.

Btw also found an issue with margin implementation when parent has fixed height. Debugging it. I have a fair idea of what's wrong but not able to pinpoint it. I hope to solve it today. Btw if you have an alternate gap implementation, I am happy to test it. We can go with whichever works best.

[jacobp100]

Yeah I've noticed an issue with 'auto' margins where they can become negative (when they should be clamped to zero). I was gonna look into it - but I suspect it'll need a feature toggle or a whole lot of layouts are gonna break

I don't think the owner is 1:1 with a parent - I assume this enabled some kind of optimisation for recalculating layouts or something like that.

[jacobp100]

@intergalacticspacehighway I've done a PR over here - #1116

[intergalacticspacehighway]

Awesome! Will test it soon 🥳

[NickGerleman]

This is now implemented (see #1116)

[jacobp100]

@NickGerleman thanks for taking this on!

[NickGerleman]

Huge congrats to @jacobp100 and @intergalacticspacehighway for pushing this through. A lot of people are very excited for it to be released 🙂. https://twitter.com/reactnative/status/1613584810223570952?s=46&t=nSxqL7Mt7r6wQFEk4snH9A