[RNMobile] Update Inserter Block Search Styling #33237
Conversation
|
Size Change: -5 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
jhnstn
force-pushed
the
update/mobile-inserter-block-search
branch
from
e2a4a5e to
7af4228
Compare
| return selectModifiedStyles( baseStyles, 'active-dark' ); | ||
| }, [] ); | ||
|
|
||
| useEffect( () => { |
There was a problem hiding this comment.
The idea here is to use a hook to build the styles on demand given any state changes. It avoids having to weave conditionals in the components per state e.g. <someComponent style = { isActive ? activeStyle : style } /> . This approach can also be used to replace the usePreferredColorSchemeStyle which needs to be called on every individual set of style rules. Instead one single call can be made on the entire style object to generate the themed styles.
I'd like to extract this way of handling the styles out to a hook since it's a more declarative way to build the style object for the component. In the interest of time however, I opted to leave this idea scoped to this module.
The signature of the hook would looks something like
const currentStyles = useModifiedStyles(allStyles,
[
{'modifierA' : [someState]} ,
{'modifierB': [someState, someOtherState]}
]
)
|
I'm concerned about the potential side effects of not having the segmented control along with the search (this only happens in dev mode), I remember when I was working on the reusable block that I experienced some issues 🤔 .
I have a fix for this issue in this PR but it's not approved yet, it would be useful to use it to test this PR as it will help to test in a similar environment as production. |
|
👋 Thanks @jhnstn for updating the styles of the search form, now it looks awesome! After testing the search functionality on both iOS and Android, I noticed a the following issue: 1. Search result list requires some padding at the bottom for the safe area [iOS][medium]I noticed this in the search result list, however, it's not happening on the regular list. Reproduced on iPhone 8 - simulator (iOS 14.2) | Medium impact
2. Search input field can be scrolled some pixelsReproduced on Samsung Galaxy S20 FE 5G (Android 10) | Low impact search-scroll-android.mp43. Some items disappear when searchingI reproduced this issue by using specific letters in the search, in the example below it's the letter Reproduced on Samsung Galaxy S20 FE 5G (Android 10) | High impact search-items-disappear-android.mp44. Debounce searchThis is just more a recommendation but totally optional, I was wondering if we would benefit from using debounce when typing the search term, I guess the logic for searching is fast enough to maybe not require it, wdyt? |
packages/block-editor/src/components/inserter/search-results.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inserter/style.native.scss
Outdated
Show resolved
Hide resolved
packages/components/src/search-control/searchFormStyles.ios.scss
Outdated
Show resolved
Hide resolved
packages/components/src/search-control/searchFormStyles.android.scss
Outdated
Show resolved
Hide resolved
|
@fluiddot thanks for the awesome review! I'd like to open up separate issues for the first 3 issues you found when testing (nice catches btw). The feature is still behind a dev flag and I feel this PR is already getting big. Thoughts? On point 4. I thought about that when I started this project but the list of blocks is static and not very large. I figure if we need to fetch blocks over the wire or if the list of blocks gets really long it would be useful to buffer the input. For now it seems like an over optimization to do that. Open to have my mind changed on that however. I cleaned up the CSS issues you noted. Regarding the logic that sets up the styles, I went ahead and extracted that code to a stand alone hook. I figure it will be easier to grok the code since it's now generalized. Overall it's the same approach as in this PR , just abstracted out and (hopefully) simplified a bit. I can cherry pick that commit into a fresh branch if we think it's worth adding as a stand alone hook. Merge that then update this branch. Thanks again for your thoughtful review! 💯 |
Make sense, let's address them in new PRs 👍 .
I thought the same, I was just wondering if we could benefit from it but I agree that it's an over-optimization as calculating the search result should be quite fast.
💯
To be honest, I don't think it's necessary if we plan to update this part after merging the stand-alone hook you mentioned. |
|
Sorry for the delays getting around to this, @jhnstn! I've tested this today and here are some comments. Android testingTested on Android emulator Nexus 6 API 29. Placeholder disappears when cancelling active searchOn Android, after typing text into the search field and then tapping the arrow button, the keyboard is closed (as expected) but the placeholder "Search Blocks" doesn't get shown in the empty search field. Subsequently tapping on the search field doesn't bring up the keyboard and I can't continue searching until I close the bottom sheet. placeholder-issue-android.movThe search field bounces up and is slow to come back downAfter opening the block inserter, I tap the search field to start filtering blocks and notice the animation slides the search field up momentarily before bringing it back down on-screen and the animation seems slow. This was seen on an emulator and also on a device. That said this seems low priority. over-bounce.movSpace between the bottom sheet drag handle and search field too tightThis is subjective and might be more a design question rather than an implementation question, but my first impression is that this spacing is too tight:
Placeholder caseThe current placeholder text "Search Blocks" stands out to me because I would have expected it to be sentence case: "Search blocks" Search field doesn't respond to changes in device font sizeAfter changing the device font size to largest, the search field doesn't adapt its height to the larger placeholder font size (text is cut-off). Tested on Samsung S10 physical device with Android 10, font size changed in device Settings app.
iOS testingI tested on a physical iPhone 11 running iOS 14.5 and quick test on an iPad simulator and things worked great. I only saw the "Space between the bottom sheet drag handle and search field too tight" and the "Placeholder case" issues. In my opinion, the only thing that seems high priority is the first issue mentioned on Android where the search becomes unusable after cancelling an active search using the left arrow button. Overall it's working great and I've been using the block inserter search a lot lately when using the editor in development. |
This behavior is on purpose because on Android, we don't get notified when the opening transition of the keyboard starts to calculate the final height of the bottom sheet. The workaround I applied for this issue was to make the transition you mentioned that happens when the keyboard transition finishes. There're further details in the following issue: #31151 |
|
Thanks @guarani for the review!
Fixed in 0ae5d0d ( also fixed the input scrolling issue @fluiddot found)
Fixed in cac6bf1 ( that was an odd one, see my inline notes on the commit ) I really don't have a strong opinion on the bottom sheet upper padding or the Sentence vs Title case for the placeholder. Both are valid points but I'll have to differ to @iamthomasbishop to decide if we should change those. |
There was a problem hiding this comment.
I've done another test on Android (on iOS everyting worked nice 🎊 ) and here are the results:
Placeholder disappears when cancelling active search
✅ Fixed
I tested on my Android device (Samsung Galaxy S20 FE 5G - Android 10) and I couldn't reproduce it.
The search field bounces up and is slow to come back down
🟡 Reproduced but it shouldn't block the PR
As I mentioned in this comment, this is a known issue that will be addressed in the future.
Space between the bottom sheet drag handle and search field too tight
❓ Wait for design feedback input
I also have the impression that this spacing is too tight. In fact, I think that's the only area of the bottom sheet where the user can swipe down to close it so maybe we should preserve the padding we had previously. In any case, I don't think this issue should block the PR.
Placeholder case
🟡 Reproduced but it shouldn't block the PR
I also expected the placeholder text to be Search block instead of Search Blocks. I saw that this change was introduced when updating this branch with trunk (commit reference), what's the purpose of changing this string?
Search field doesn't respond to changes in device font size
✅ Fixed
✅ I verified that the issue I mentioned in a previous comment ("Search input field can be scrolled some pixels") was also fixed.
I tested with the biggest font size on my Android device (Samsung Galaxy S20 FE 5G - Android 10) and the cut-off is addressed.
Apart from the tests, I added a comment in the code with a suggestion, it's just a minor change so feel free to skip it.
From my side the PR LGTM 🎊 but it would be nice to have the approval from @guarani too, thanks 🙇 !
I also went ahead and changed the case for the other labels and hints in the component. |
There was a problem hiding this comment.
I re-tested Android and iOS and don't see any of the issues I noted earlier in #33237 (comment), except those that @fluiddot noted here (which I understand are not meant to be addressed here).
Description
This updates the inserter block search styling to handle variances between iOS and Android. It also introduces a no results view.
How has this been tested?
asdfasdfuntil onlyasremains.Screenshots
Open Inserter
Start Search
No results
Types of changes
This adds new mobile platform specific style sheets and a new native component to show the no results view.
Checklist:
*.native.jsfiles for terms that need renaming or removal).