[RNMobile] Full width columns #25621
Conversation
lukewalczak
force-pushed
the
rnmobile/full-width-columns
branch
from
be28ce5 to
b981944
Compare
lukewalczak
force-pushed
the
rnmobile/full-width-columns
branch
from
b981944 to
73b2181
Compare
There was a problem hiding this comment.
Tested it on iOS and Android and everything works. Great job @lukewalczak !
There was a problem hiding this comment.
Hey @lukewalczak 👋
Awesome job working on this big task! 👏 👏 It definitely wasn't an easy one dealing with the alignments.
I gave another quick look at the code and just left two minor comments.
The great news is that I tested this again on an iPad, iPhone, and Android and I think this PR is ready to be shipped (@iamthomasbishop to have the final word on this on the design perspective) but all of my previous concerns regarding paddings are now solved 🎊
I tested different cases and also some Home page layouts:
|
Gave it a quick run, it looks really good on landscape but on portrait I did notice something related to Gerardo's comment about button's margins. Tested on Android with a Nokia 8 (1440x2560): |
Could you please confirm you are using the latest changes? Does it happen in WP-Android app as well? |
There was a problem hiding this comment.
Did a product review and it worked as advertised for me 🎉 Awesome work here @lukewalczak
|
After approves:
|
|
@lukewalczak As discussed in dm, while there are still some minor spacing things I think we could tidy up I think we should merge/ship this and iterate** on it after the break. Great work, Luke! 👏 ** We are planning on re-thinking the way spacing, borders, etc. are structure in the near future, so the aforementioned spacing concerns should be addressed as part of that work) |
These styles caused columns to align to the center when their width sum did not equal or execeed 100%. Removing these styles did not appear to negatively impact other inner blocks or the use cases outlined in #25621.
* test: Await overlooked asynchronous task The test helper executes asynchronous updates to the layout. If this is not awaited, test failures can occur in certain circumstances. * feat: Combine BlockList and BlockListCompact Reduce code duplication between the separate components, e.g. item render, footer, end-of-list block appender button, empty list placeholder. * wip: Gallery, Columns working; Buttons broken Partially fix multi-column layouts by passing existing styles to a wrapping view. All of the passed styles may not be necessary, need to investigate. Also, Buttons render broken, wrapping lines and overflowing the parent container. * test: Fix nested lists tests Nested lists now rely upon `BlockListItem`, which returns `null` if the `blockWidth` has not yet been set. In order for test queries for nested list items to succeed, we must trigger the `onLayout` callback for the nested block lists. `BlockListItem` is now used to expand capabilities for nested blocks, e.g. multi-column grid items. * wip: Fix Blocks button layout Fixes Buttons, but Columns remain broken. * refactor: Remove unused BlockList title prop * refactor: Separate inner block list styles The list is no longer shared, so we can merely set the appropriate styles on each list element. * refactor: Remove unused virtualized listKey Now that inner blocks do not use a virtualized list, a unique list key is no longer necessary. * refactor: Remove outdated inline comment The scroll ref is no longer passed to inner blocks as they do not use virtualized lists. * wip: Explore nest Columns fixes The relocated styles may need to be separate from the top-level block list element, as the styles may conflict with other styles. It does not fix Columns, however. * Mobile - BlockList - Fix layout issues for stacked horizontally blocks * fix: Revert Buttons alignment workaround This caused issues for non-Buttons inner block alignment. The original issue of Buttons inner blocks not rendering inline was addressed in 43c0b14918f8ed03037c01d94321922dc31a7fa3. * fix: Column width sums less than 100% correct align These styles caused columns to align to the center when their width sum did not equal or execeed 100%. Removing these styles did not appear to negatively impact other inner blocks or the use cases outlined in #25621. * refactor: Inner blocks use clientId key This mirrors the approach for cells of top-level block lists. It is also likely more robust, providing better performance when reordering blocks. * refactor: Remove unnecessary key The inner blocks list now sets a key on a wrapping view, rather than via the `renderItem` function. Thus, this key is no superfluous. * fix: Prevent block list footer from stretching to parent height The parent flex styles were applied to the footer element. This wrapping view prevents those styles from erroneously stretching the provided footer element. * Mobile - List block - Remove usage of useCompactList and disables the option to render the appender for InnerBlocks * test: Update Android e2e Buttons block Xpath selector This Xpath selector became outdated with the block list refactor. * test: Delay query for block actions menu Appium reported this cached element had been removed from the DOM. Relocating the query seemingly resolved this issue. --------- Co-authored-by: Gerardo <[email protected]>
Description
That PR is adding full width (along with different width unit) functionality to
Columnsblock.How has this been tested?
full widthExpect that columns are equal and always stacked when portrait mode (width < 480)
Expect that columns are respecting the passed width when landscape mode
Expect that columns are equal and are max 3 in a row when landscape mode
Expect that nested columns block behaves accordingly to the rules/ breakpoints above
Testing HTMLs
Screenshots
Types of changes
Checklist: