-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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] Latest Posts Block v1 Support in Mobile #20301
Conversation
Size Change: +226 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
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 tested via the steps in the description, and everything works as described. Nice work 🎉 !
Minor note (not introduced by this PR):
While testing this, I noticed an issue with the bottomsheet: The PanelBody
titles do not allow scrolling. Instead, a drag-up does nothing, and a drag-down begins to dismiss the bottomsheet.
Screencast:
I'm not sure if this is a known issue?
Yeah, good call out. I believe this is a known issue because @iamthomasbishop had mentioned it to me in slack. I think this is tracked by this issue wordpress-mobile/gutenberg-mobile#1884 |
font-size: 14; | ||
font-weight: 600; | ||
color: #2e4453; |
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.
Curious, why do we hard code this in place of the sass var?
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 had made some design tweaks and I was aligning to what’s in the media placeholder. That used the hex color instead of a var and when I searched it looked like this color hasn't been transferred to a var yet. So I decided to stick with the current pattern for now.
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.
Ah, this one was using the darken
function, so might have been harder to spot: https://github.com/wordpress-mobile/gutenberg-mobile/blob/7500e0eb4d62279659c3af100c1311d255e099a7/src/_colors.scss#L98
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.
Ah yeah that one didn't pop up on my search. I'll make a PR to fix that.
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.
LGTM! Great work @chipsnyder !
Tested on the main iOS app and ✅
On the demo app it shows a red screen when opening the settings though, pretty sure its related to the categories but I guess it's not a blocker.
That actually sounds odd to me. I didn't test in the demo app but mocked failed network requests and didn't get a red screen. The user should just see "All" in the categories. I'll take a look and make sure there isn't a different object or status code being returned by the demo app that a user might see. Good call out! |
I'm glad you checked that @geriux! The demo app was returning an object that wasn't an array as a result of its failure where in my other testing the request would fail and fall through to the catch block. So I added some additional checking to make sure we have a non-empty array. |
parsedCategoriesList = JSON.parse( categoriesList ); | ||
} | ||
|
||
if ( isEmpty( parsedCategoriesList ) ) { |
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.
The demo app was returning an object on its failure to fetch categories. I tried to add typeof categoriesList === 'array'
here but got a lint error. So instead I'm checking to see if it's empty which seems to handle the weird object case.
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 didn't look much into the code as this has already one approval, but I tested on Android with a bunch of different combinations of settings, and it seems to be working great 🎉
Thank you! Just tested it again on both the demo app and the main app and it's working correctly. |
import { __ } from '@wordpress/i18n'; | ||
import { postList as icon } from '@wordpress/icons'; | ||
import { InspectorControls } from '@wordpress/block-editor'; | ||
import { fetchRequest } from 'react-native-gutenberg-bridge'; |
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 shouldn't be used directly and instead, it should use the GB apiFecth
interface. This will provide support for retries and cache of the data.
The only thing needed is to add the /wp/v2/categories
to the white list here: https://github.com/wordpress-mobile/gutenberg-mobile/blob/e288c45e882db9808e0396715f379edb07f32416/src/api-fetch-setup.js#L44
@chipsnyder if you need help please ping me or @etoledom.
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 @SergioEstevao! I'll make a PR either today and push it up.
Fixes: wordpress-mobile/gutenberg-mobile#1746
Related PRs:
gutenberg-mobile
Latest Posts Block v1 Support in Mobile wordpress-mobile/gutenberg-mobile#1931WordPress-iOS
[Testing] RNMobile Latest Posts Block wordpress-mobile/WordPress-iOS#13495WordPress-Android
[Testing] RNMobile Latest Posts Block wordpress-mobile/WordPress-Android#11338Description
Adds v1 support for the Latest Posts Block on mobile.
The block in the editor looks similar to an unsupported block but with the word "Customize" instead of unsupported.
After the block is selected you can open the settings menu by:
In the settings, you can configure the following settings:
excerpt
and true translates tofull_post
Show full post content
is falseHow has this been tested?
These tests were validated via WPiOS and WPAndroid
Categories make a network request to fetch the available categories, so they were tested against a Wordpress.com site as well as a self-hosted site.
Add New Block
Expect: block to be added
Expect the settings to show
Validate the block is configured based on the settings in the table above
Modify Block
This should be tested by modifying in mobile and validating on the web and by modifying on the web and testing on mobile.
Conditional items:
true
should hide "Max number of words in excerpt"false
should show "Max number of words in excerpt" with the last selected valueValidate by checking the page on the other platform and by going into the same page/post that all settings match how you configured them.
Categories known behaviors
Categories are fetched in
componentDidMount
which results in a flow where multiple Latest-Posts Blocks can show different data.- Take Note of the Categories -
- Note the first block does not reflect the changes in the categories -
- Note the second block does reflect the changes in the categories -
This behavior is consistent on the web and mobile. Leaving the page and coming back will sync the categories.
Screenshots
Types of changes
Checklist: