-
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
Add/block preview #26346
Add/block preview #26346
Conversation
@@ -204,11 +204,22 @@ example: { | |||
'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Praesent et eros eu felis.' | |||
), | |||
}, | |||
}, | |||
}, |
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.
should we indent the whole object here using tabs? (otherwise, we might want to keep this for a separate PR and revert this line)
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.
Done
@@ -30,6 +30,8 @@ Width of the preview container in pixels. Controls at what size the blocks will | |||
|
|||
`viewportWidth` can be used to simulate how blocks look on different device sizes or to make sure make sure multiple previews will be rendered with the same scale, regardless of their content. | |||
|
|||
`viewportWidth` can be override in example object with the property `viewportWidth`. |
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.
BlockPreview component can be used in very different contexts, I don't think we should be mentioning the "example" object here. Can we leave this out?
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.
Done
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 sorry, I was not clear enough, I was thinking we should remove the whole sentence from here.
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.
OK it's done
( hoveredItemBlockType.example && | ||
hoveredItemBlockType.example | ||
.viewportWidth ) || | ||
500 |
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.
Potentially you could have written hoveredItemBlockType.example?.viewportWidth ?? 500
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.
Done
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 for the PR this looks like a nice API.
You could have expanded a bit more on the PR description to help reviewers understand and test the PR.
Thanks for review. Sorry for the lack of description, it's my first PR. But I take note for the next one |
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.
This is looking good. Thanks for the addition.
Congratulations on your first merged pull request, @jeff-be! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
Description
closes #26289
How has this been tested?
Screenshots
Types of changes
Checklist: