CSS Grid 2/9: Grid layout algorithm#1894
CSS Grid 2/9: Grid layout algorithm#1894intergalacticspacehighway wants to merge 3 commits intofacebook:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
facebook-github-bot
added
the
Shared with Meta
Summary: Add the core grid layout computation and integrate it into the layout dispatcher. Includes: - AutoPlacement.h: auto-placement algorithm for grid items - GridLayout.h/cpp: grid layout entry point - TrackSizing.h: track sizing algorithm - CalculateLayout.cpp: grid dispatch block and #include - CMakeLists.txt: add algorithm/grid/*.cpp glob - React Native mirror of all C++ changes Differential Revision: D93946253
intergalacticspacehighway
force-pushed
the
css-grid-2-algorithm
branch
from
76d64ff to
083f895
Compare
NickGerleman
left a comment
NickGerleman
left a comment
There was a problem hiding this comment.
Been looking through this, for about an hour.
I think we still need to chunk this change into steps, of smaller concerns, to be able to give it a real review (vs trivial nits) which I feel like I ought to do, if we want to maintain this long term. It's been tricky for me to understand how everything connects. That might be an indication, it's worth documenting some of the newly added structures, if what they're doing isn't intuitive.
Could we build this up, in small parts of spec/behavior/structures at a time?
|
|
||
| namespace facebook::yoga { | ||
|
|
||
| struct GridTrack : GridTrackSize { |
There was a problem hiding this comment.
This organization is confusing to me. Why should style input, and calculations about it, have to be same/inherited object? We have two GridTrack.h, where the most public one is for GridTrackSize (maybe that should be GridTrackSize.h?).
|
|
||
| namespace facebook::yoga { | ||
|
|
||
| struct TrackSizing { |
There was a problem hiding this comment.
Keeping all this logic, in a single header, in a nested struct, without header explaining purpose of structure, is pretty hard for me to parse. Esp since everything is public. Is this to try to encapsulate since header only?
I think bc header only, we would also be needing to mark things as constexpr or inline.
| float containingBlockHeight; | ||
| }; | ||
|
|
||
| using CrossDimensionEstimator = std::function<float(const GridItem&)>; |
There was a problem hiding this comment.
nit: we should avoid using std::function unnecessarily, compared to underlying functor type. It introduces overhead like heap reallocating capture group.
| namespace facebook::yoga { | ||
|
|
||
| struct OccupancyGrid { | ||
| std::unordered_map<int32_t, std::vector<std::pair<int32_t, int32_t>>> |
There was a problem hiding this comment.
std::unordered_map is quite expensive, heap allocating a node per inserted item. How many items is this usually be? Would it be better as a flat vector of items/binary searched vector? (sidenote, this is what std::flat_map is kinda meant to solve).
Sure @NickGerleman. yeah i thought this might be needed 😅. Will split it and also address above mentioned points in them. |
3 participants
Summary
AutoPlacement.h,GridLayout.h/cpp,TrackSizing.hCalculateLayout.cppgrid dispatch,CMakeLists.txtupdatesSplit from #1865. Depends on #1893. Merge after 1/9.