-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RNMobile - Cover Block: First iteration #19722
Conversation
/** | ||
* External dependencies | ||
*/ | ||
import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't import React directly. I think everything you might need from React should be available in @wordpress/element
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Updated it to use @wordpress/element
instead =)
Hey @geriux, testing on Android worked well for me. I did notice one thing that was inconsistent between web and mobile, but I'm not sure of the desired behavior or scope. In mobile, I can set
|
Thanks for testing @chipsnyder! I'll have a look at that issue, we should prevent that as the web does. |
…if the content has higher height
@chipsnyder Updated the code and now it should be working as the Web. Let me know, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @geriux!
@@ -156,7 +156,7 @@ function ResizableCover( { | |||
); | |||
} | |||
|
|||
function onCoverSelectMedia( setAttributes ) { | |||
export function onCoverSelectMedia( setAttributes ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK, but I'm wondering if it'd be better to extract this logic to a shared utils
file instead, and have a attributesFromMedia
function there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to extract it, especially since the web file is kinda big already. a4dfbdd thanks for the name suggestion!
const INNER_BLOCKS_TEMPLATE = [ | ||
[ 'core/paragraph', { | ||
align: 'center', | ||
fontSize: 'large', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned about this, since we still can't render or change font sizes from the app. This might lead to the user publishing something quite different from what they're seeing on mobile. This is specially worse if they add more than one paragraph, since the first one will be large by default and the second won't, but they would look the same inside the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't think about that 😱 good catch! For now, I'm removing setting the attribute as default until we support font sizes 5305373
} | ||
|
||
if ( url ) { | ||
return dimRatio !== 0 ? dimRatio / 100 : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, how is this different than just doing return dimRatio / 100
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seriously can't recall why I did this 😆 but yeah no need for that, updated it here e06c903 (sorry this commit has more changes due to prettier formatting)
return dimRatio !== 0 ? dimRatio / 100 : 0; | ||
} | ||
|
||
return url ? 0.5 : 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an if ( url )
with a return
above this, so this would always return 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup no need for this, updated it e06c903#diff-b17d53802519ed746057fb2ee4a915edR111
|
||
const ImageWithFocalPoint = ( { | ||
containerSize, | ||
contentHeight, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly from previous experiments, you can make the image fill the container with absolute positioning without knowing the exact container dimensions in code. We would still have to listen to layout events and get the exact dimensions to calculate offset, but it might be better to keep that logic inside this component than passing dimensions from the outside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely better to have the logic within the component, simpler to just pass the url
and focalPoint
and let the component handle the rest 🙌 e71ee48
…/cover-block # Conflicts: # packages/block-editor/src/components/media-upload/index.native.js # packages/components/src/index.native.js
… solid background
Thank you for the feedback!
This looks like is a bug that we have in develop, I'll look if there's an opened issue if not I'll open one, thanks for spotting that!
I added both options =)
Good catch! This case was when selecting a color from the theme. Now It will show a black color until we have support for that unless you have another color in mind =)
This was related to the previous point, it is fixed now.
I'll investigate this since I tried it in the
Sounds good! |
Awesome! Thank you.
👍
Ahh, that’s right, I hadn’t thought about the theme-color thing affecting this. Perhaps for now we should use the same dark gray color as the placeholder BG.
I should have been clearer in my messaging — this is something that we don’t need to worry about as it’s a separate issue. I think it’s a refinement that is either being addressed or will be in the near future 😀 Thank you, @geriux! This is really coming along nicely! |
Thanks for spotting this! Fixed by a61cbb6#diff-b17d53802519ed746057fb2ee4a915edR118 |
Thanks for testing this again @chipsnyder !
Did you have the latest changes? I thought I fixed this and I can't reproduce it =(. Could you check if you had the latest commits? If you have, can you tell me the steps to reproduce?
This should be fixed too (same as above) Regarding the colors, I'll check again. I thought there were some default colors that we did support but it looks like that isn't working anymore. I'll let you know =) For theme colors though, since we don't support those we will show the placeholder background until we add that feature. |
Hey @geriux, I was able to reproduce these, but I put my block config below and the steps. For the first one, I might just be using an unsupported color option. When starting my metro server, I tried starting it with Device: Pixel 3a API 29 (Physical Device) Cover with BG Color
This block is using a color from the theme. I used the same color for user case 3, so it may be the same problem. Are we supporting theme colors yet? The theme for my test site is Maywood. Some placeholder backgrounds overflow the container Block Config:
In Web: In Android: |
Thanks for double checking @chipsnyder ! You had the latest changes and with your latest feedback, I was able to reproduce the issues =)
Oh ok yeah not yet, only custom colors for this iteration (I'll update the test case) Regarding the placeholder overflowing the margins, should be fixed with the latest commit 7a9341c 🙌 |
I had one question on the link text in the paragraph block:
If I preview the page and use that default link this is what that looks like
Editing thanks @geriux for letting me know about my mistake |
Thanks for testing! Glad to see those items are working correctly now =) Regarding the links, there's an open issue related to it. Looks like it is not detecting links very well when it checks the clipboard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment around some code style, but it's not that important. Overall this looks great ✨
…/cover-block # Conflicts: # packages/block-editor/src/components/media-upload/index.native.js # packages/block-library/src/index.native.js
Thank you all for the feedback! 🚀 |
Size Change: +368 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
@@ -12,6 +12,10 @@ import Paragraph from '../edit'; | |||
* WordPress dependencies | |||
*/ | |||
jest.mock( '@wordpress/blocks' ); | |||
jest.mock( '../../../../data/src/components/use-select', () => () => ( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some tests trying to mock this, using requireActual
, etc. I wasn't able to make it work due to some exports issues.
We can't use @wordpress/data
as the path due to the symlinks we have in Gutenberg Mobile
There were some tests in another mock here #19705 (comment) without any luck.
I'll keep an eye after this is merged to see how we can improve this.
Guntenberg Mobile
PR -> wordpress-mobile/gutenberg-mobile#1781Description
This is the first iteration of the Cover block including:
For the focal point implementation, I created a new component
ImageWithFocalPoint
which allows passing the coordinates and setting the image in the right position depending on its container size.The default text color was an issue for
InnerBlocks
because we don't have a system to inherit styles from parents. Our current default color is a darker one, the default text color on Web for any block within aCover
block is white. So I did this approach but I'm not sure if its the best one, any suggestions would be appreciated. The idea is to set a default text color for the parent block in this caseCover
then theRichText
component will check if there are styles set by its block parent. If not it will use the color from the style prop or as last option the default color.I added a new prop
__experimentalOnlyMediaLibrary
forMediaPlaceholder
andMediaUpload
to show only the Media Library option while we develop the rest implementation of theCover
block.Updated the
Paragraph
andHeading
blocks to use__experimentalUseColors
and to add some missing props.Screenshots
How has this been tested?
User case 1
Cover
blockAdd Image
WordPress Media library
Minimum Height in pixels
andBackground Opacity
User case 2
Focal point
picker in the right sidebar, select any point.Cover
block to be visible, with the image, focal point, and text color selected.User case 3
Cover
blockCover
block to be visible, with the Heading block inside the block.User case 4
Media Settings
boxCover
block to be visible (since we removed the background image), respecting the styles (margins, paddings, etc)Types of changes
New feature (non-breaking change which adds functionality)
Checklist: