[RNMobile] Reimplementation of block borders #20769
Conversation
dratwas
changed the title
|
Size Change: 0 B Total Size: 860 kB ℹ️ View Unchanged
|
dratwas
changed the title
There was a problem hiding this comment.
Masterpiece! Seriously! 🎉 Can't describe how happy I am to see lots of code removed.
I have detected 3 issues among below tests but in general I think no big issues here. I only tested with iOS.
@jbinda Could you test with Android as well?
-
Media Text block - Media & Text alignment
-
Media Text block - Vertical alignment
-
Group - AppenderButton is rendered - steps
-
Group - Deep nesting is possible (iOS only) - steps
-
Group - Check if Group placeholder is visible for the unselected state - steps
-
Group - Check if Group placeholder is render in nested structure - steps
-
Group - Nested block have proper border styling - steps
-
Group - Nested block have proper margins values - steps
-
Group - Breadcrumbs on FloatingToolbar is properly displayed - steps
-
Group - Navigation up button works as expected - steps
-
Group - Navigation down works according to parent-first approach - steps
-
Group - Cross navigation between blocks works as expected - steps
-
Group - Ungroup button works as expected - steps
-
Group - Check if in DarkMode all components gets proper colors (iOS) - steps
-
Group - Check if nested Placeholder block can be replaced - steps ❌ <<< I saw the issue in this PR while testing it, was that fix merged into this branch?
-
The empty space under toolbar, see code comments
-
Media-Text: The extra space between media - text, see code comments
packages/block-editor/src/components/block-list/block.native.scss
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/block.native.scss
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/index.native.js
Outdated
Show resolved
Hide resolved
|
@pinarol sure I will test in on Android today in 1st place
I see that is should contain this fix. I will double check because I didn't noticed any issue when testing this fix before merge |
|
I can't repro this anymore, it should have been fixed by the latest merge from master by this PR.
|
|
Tested on Android demo and WP Android:
Media Text block - Vertical alignment - question Is below screen present proper item alignment for Horizontal layout when vertical alignment is set to top ? (the dashed border frame is not align with image)
Apart of above checklist I have noticed another thing with Separator line. Depends on hierarchy level the Separator line has different width. Not sure if it is expected or something brings by this PR. However maybe it is worth to fix this here to make Separator behaviour consistent with other changes. This is question to @iamthomasbishop probably but shouldn't the Separator line align to content as well ? See below screens for details.
|
I think so, apart from the space on left and right (already fixed) |
Those 2 work the same way on prod. If there's something that needs to be improved about these items we can handle it on a different PR since this PR's purpose is only improving the code quality while making sure that we aren't breaking anything. |
|
Cool ! That was the only findings that I was able to spot during my todays testing |
pinarol
left a comment
pinarol
left a comment
There was a problem hiding this comment.
I repeated my tests on iOS, issues I reported are fixed. I can't detect any regressions this PR is causing. I think this is ready to go. Thank you for all the great work! Again, masterpiece!
@jbinda please give 👍 for Android side if you are finished with testing.
jbinda
left a comment
jbinda
left a comment
There was a problem hiding this comment.
I have tested on Android and also do not noticed any regression.
Let's ship it :)
packages/block-editor/src/components/block-list/block.native.scss
Outdated
Show resolved
Hide resolved
|
hooray 🚀 |
Description
In this PR i reimplemented borders of blocks according to that issue wordpress-mobile/gutenberg-mobile#1948
Gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2050
I removed old borders and calculations related to them and set borders as a view that is positioned absolutely.
I also had to manage all margins because they changed after removing borders. I mentioned a few issues related to that here. I fixed them by using negative margins in the parent (FlatList) except the root list.
So the base concept is - if the parent has margin set to -16 and each inner block has margin set to 16 then the edge margins are cut and we don't need to check if it is first/last block etc. This solution should work for horizontal blocks as well w/o any calculations. Tested on columns and buttons.
I also added a possibility to change the horizontal or vertical margin of inner blocks since @lukewalczak said he would need that probably in buttons block.
After these changes, I had to fix margins in cover, media, and group block.
NOTE:
This PR needs an accurate review of spacing of borders and inner blocks. @iamthomasbishop could you please check it? :)
How has this been tested?
All tests cases related to the spacing and borders from here:
https://github.com/wordpress-mobile/test-cases/blob/master/test-cases/gutenberg/group.md
Basically the editor should look the same as before :)
Screenshots
Types of changes
Reimplementation of blocks borders
Checklist: