[Mobile] Link picker via fetch #23922
Conversation
This should not be necessary, and there should be a better way to achieve the performance benefits from a flatlist nested with the same orientation of an ancestor, especially when the scroll dimension can be fixed.
This reverts commit c8adbb8.
… into rnmobile/feature/link-picker-via-fetch-without-navigation
mkevins
added
[Type] Enhancement
|
Size Change: +207 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
…link-picker-via-fetch-without-navigation
There was a problem hiding this comment.
Looking good! Nice work to all involved in this feature!
Just left a small comment.
I do have one question regarding the behavior on an iPad. Should the bottom sheet reach the top? or have some sort of max height?
Edit: Well actually I just noticed I had the keyboard hidden so when it's visible it doesn't look that big of a space =D
|
@geriux great catch! I had missed this because I mostly use landscape when on iPad (usually w/ a hardware keyboard) so this is a good reminder to test in portrait more often 😄 ). I agree the height is a bit awkward, but I don't think it's necessarily just a matter of applying a max-height (although that would probably help). I think we should attack this across the board, for tall sheets like in the editor, not necessarily in this specific work (unless y'all think it's an easy-enough fix to include here, of course). In terms of solution, I think we should do something similar to iOS' standard modal sheets, which use following general rules (on tablet):
Examples:
|
Hey @iamthomasbishop! Thanks for giving examples of a better UI using modal sheets for iPad! Since that involves a bit more work/investigation and it's not entirely related to this PR let's work on it as a separate task and move forward as it is now. |
Thanks for the suggestion! I've created an issue to track it. |
lukewalczak
deleted the
rnmobile/feature/link-picker-via-fetch-without-navigation
branch
There was a problem hiding this comment.
👋 Hi! Via #49900 (comment), I found that there is a missing dependency in a useSelect usage in this PR. I'm not a mobile developer, I'm thinking maybe one of you is in a better position to help fix and test this issue.
| return { | ||
| fetchMoreSuggestions: debounce( fetchMore, REQUEST_DEBOUNCE_DELAY ), | ||
| }; | ||
| }, [] ); |
There was a problem hiding this comment.
hasAllSuggestions should be in the useSelect dependency array.
There was a problem hiding this comment.
Thanks for the comment ecgan 👍
It's been a while since I've worked with this code / codebase - but I wonder if simply adding hasAllSuggestions to the array might introduce some kind of regression 🤔 . Since fetchMoreSuggestions is used in the reset useEffect, and is currently expected to be stable, it may need to be added there as well - though I'm not entirely sure off-hand if that can lead to any issues, since the function is debounced (e.g. will this break debouncing in some cases?)
If this is to satisfy a new lint error, and is blocking that work, maybe the simplest solution is adding an ignore - otherwise, this would need further testing in a mobile build to ensure it doesn't introduce regressions, imo.
There was a problem hiding this comment.
@mkevins , thanks for getting back to me. 🙏
I wonder if simply adding
hasAllSuggestionsto the array might introduce some kind of regression 🤔 .
Yes, that's the same concern I have too.
maybe the simplest solution is adding an ignore
Yeah, I think we can do that. I have just done that in #49900 (comment).
Related PRs
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#2484WordPress-Android: wordpress-mobile/WordPress-Android#12438WordPress-iOS: wordpress-mobile/WordPress-iOS#14537Details
This is a PR to produce a test apk for design review of the link picker feature.
To test:
Follow testing steps here: wordpress-mobile/gutenberg-mobile#2484 (comment)
Checklist: