[RNMobile] Add featured image settings to the Latest Posts block #39257
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
With this commit a 'display featured image' toggle is added to the Latest Post block's settings. This has been added to a new 'Featured image settings' panel.
SiobhyB
added
[Type] Enhancement
|
Size Change: +5 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
added 7 commits
March 7, 2022 15:03
The new 'link to featured image' toggle that's added with this commit will only be visible when the 'displayFeaturedImage' toggle is set to true.
With this commit, the 'BlockAlignmentControl' component is used to add alignment settings under the Latest Post block's featured image settings/section.
Prior to this commit, the BlockAlignmentControl component was only set up to display as a toolbar button on native. With this commit, a "isBottomSheetControl" prop is introduced to enable the component to be styled in a way that's appropriate within BottomSheets.
SiobhyB
added
the
Mobile App - Automation
fluiddot
reviewed
Mar 23, 2022
There was a problem hiding this comment.
@SiobhyB thanks for adding the featured image settings to the latest posts block, great work 🎊 .
I tested locally and the changes work as expected, however, I'm wondering whether to use the dropdown for displaying the alignment options. My concern comes from the fact that for other settings like colors, this is done within a subscreen instead of displaying another bottom sheet over the main one. The approach you followed is ok from my side, but in order to keep consistency with the rest of elements within block settings, we might consider that option, wdyt?
I understand that the change I'm suggesting implies applying several changes to the PR, so please don't take it as a mandatory change but just as a suggestion. I'll be more than happy to discuss the best approach and if eventually, we consider continuing with this PR I'll be more than happy to approve it. I'd like also to have a second opinion from another reviewer regarding using a subscreen vs bottom sheet, @jhnstn what's your opinion?
|
@fluiddot, thank you for your review! I think you're right. I was pretty focused on re-using the Edited to add: I've gone ahead to refactor using the |
added 4 commits
March 24, 2022 20:37
As per the following feedback, this commit reverts the implementation of BlockAlignmentControl in the Latest Posts block: #39257 (review)
Makes sense to me, I think the implementation was right but my main concern was about consistency.
Great ⭐, thanks @SiobhyB for updating the PR 🙇 ! I like the idea of using the |
fluiddot
reviewed
Mar 29, 2022
Comment on lines
210
to
215
| <BottomSheetSelectControl | ||
| label={ __( 'Image alignment' ) } | ||
| options={ BLOCK_ALIGNMENTS_CONTROLS } | ||
| onChange={ this.onSetFeaturedImageAlign } | ||
| value={ featuredImageAlign } | ||
| /> |
There was a problem hiding this comment.
In the future, it would be great if BlockAlignmentControl renders this component (i.e. BottomSheetSelectControl) underneath, this way we won't need to set up the options and we could use it here.
I was checking the previous commits where you introduced isBottomSheetControl prop in BlockAlignmentUI, and I was wondering if we could follow that approach for this. But, I noticed that it most likely imply a big refactor because we would need to implement the native version of the Block Alignment Toolbar component to provide here the BottomSheetSelectControl component.
There was a problem hiding this comment.
Great, thank you very much @SiobhyB for tackling this 🙇.
I'd like to clarify that with my previous comment I didn't want to imply that we had to do the refactor in this PR, but I'm happy to see you sorted it out 😊. I'll try to take a look as soon as possible at the latest changes.
Comment on lines
16
to
25
| export const BLOCK_ALIGNMENTS_CONTROLS = [ | ||
| { value: undefined, label: __( 'None' ), icon: alignNone }, | ||
| { value: 'left', label: __( 'Align left' ), icon: positionLeft }, | ||
| { | ||
| value: 'center', | ||
| label: __( 'Align center' ), | ||
| icon: positionCenter, | ||
| }, | ||
| { value: 'right', label: __( 'Align right' ), icon: positionRight }, | ||
| ]; |
There was a problem hiding this comment.
Not sure if we should expose a native-only constant in the constants file, if it's going to be used only in the edit.native.js file I might consider defining the constant there, wdyt?
There was a problem hiding this comment.
The changes to this shared constants file have been removed following the recent refactor. 👍
|
@SiobhyB I also noticed that the PR checks look frozen, maybe updating with |
added 7 commits
April 22, 2022 22:57
With this commit, the 'BottomSheetSelectControl' component is replaced to make use of 'BlockAlignmentControl'. Any code that relies on 'BottomSheetSelectControl' is also removed.
As more native-specific changes are going to be added to 'ui.js', a native version is created with this commit.
Prior to this commit, the BlockAlignmentControl component was only set up to display as a toolbar button on native. With this commit, a "isBottomSheetControl" prop is introduced to enable the use of the "BottomSheetSelectControl", so that the alignment control can be used within BottomSheets.
Following the separation of native-specific code to its own file, this commit further refines some unnecessary code that was left behind in the (now) web-only file. Specifically, the logic surrounding the 'isCollapsed' prop previously included a check for whether 'isToolbar' is true. Now that there's a native-specific file, 'isCollapsed' will only ever be passed when 'isToolbar' is true. So, the extra check is redundant.
As 'popoverProps' and 'toggleProps' are only passed/used when the toolbar component is used, this commit moves them from the 'commonProps' variable.
|
@fluiddot, thank you for your suggestions! I've gone ahead with a refactor, so that |
|
Thanks so much @fluiddot! 🙇♀️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes wordpress-mobile/gutenberg-mobile#4034
Partially fixes #29815
Related PRs
gutenberg-mobile:[RNMobile] Add featured image settings to the Latest Posts block wordpress-mobile/gutenberg-mobile#4674Description
This PR adds featured image settings to the native Latest Posts block. This is a step towards bringing the available block settings on the native side in alignment with the web. If a user selects the new
Display featured imagetoggle, the following two options for customizing the featured image's appearance will be unveiled:Note, the option to specify the featured image's size/dimensions will be addressed separately to this PR. This is because these options are a bit more complex and may warrant some updates to the image block's settings.
Please refer to the
Types of changessection below for an overview of the code changes.Testing Instructions
Testing changes in the app
Prerequisite: A WordPress site with more than one published blog post, with a featured image assigned to at least one of the posts.
Testing for any web regressions
As these PR introduces some code changes to the web's alignment controls, there is a chance for unintentional regressions. We should therefore test that the alignment controls continue to work as expected while checked out with this branch on the web. Places where the controls appear include within the settings for the Image and Latest Posts blocks.
Screenshots
Video
featured.settings.mov
Types of changes
New feature (non-breaking change which adds functionality), with the following high-level overview of the code:
Featured image settingssection has been added to the Latest Posts settings, following the same code pattern used for other sections.Display featured imagetoggle, the following two options for customizing the featured image's appearance will be unveiled:BlockAlignmentControlcomponent to implement the alignment options, which required the following changes:BLOCK_ALIGNMENTS_CONTROLS.BlockAlignmentControlenable theBottomSheetSelectControlcomponent to be rendered when theisBottomSheetControlprop is set totrue. This was a desired change, as it'll make it easier to add alignment options within a block's bottom sheet settings (rather than only the toolbar) in the future.BottomSheetSelectControlwas updated to allow icons to be displayed for each option in the subsheet, enabling the use of the alignment icons. It's likely that the use of icons will be a future use case for other developers using this component, too.Checklist:
*.native.jsfiles for terms that need renaming or removal).