🐛 (expo): Fix various RTL reader issues#786
🐛 (expo): Fix various RTL reader issues#786aaronleopold merged 10 commits intostumpapp:breaking/sea-ormfrom
Conversation
Arklaum
changed the title
aaronleopold
left a comment
aaronleopold
left a comment
There was a problem hiding this comment.
As usual, this looks great! Thank you for consistently making this app better and better!
I saw some TODO's that said swap to FlashList for performance improvements, but they don't have any inverted prop (which the RTL logic currently relies on and I don't feel like changing it unless I have to)
They do have guidance for migrating usages of inverted from FlashList v1 to v2: https://shopify.github.io/flash-list/docs/v2-migration#step-4-replace-inverted-lists. It does mention manually reversing the data set which is an unfortunate burden on us (but understandable from their perspective I'm sure haha).
I also saw another comment somewhere else about LegendList and those do have an inverted prop, so I might give that a go.
I did try LegendList before really landing on FlashList and had lots of awkward recycler issues from what I remember, but especially on Android. I think they also released a v2 around the same time as FlashList, though, which I haven't tried yet
I also updated my little list to be easy to tick completed stuff: #770 (comment)
❤️
I'm curious about if you look at each commit separately or the whole thing at once?
I usually give the commits a glance before digging in, but I mostly just look at the whole thing all at once. I think having clean, smaller commits like this makes it really convenient if I encounter something that is easier to process in a reduced scope (i.e., just the commit) but the review tools GitHub provides makes the review overall easier all at once
| const getSliderValue = useCallback( | ||
| (idx: number) => { | ||
| if (readingDirection === ReadingDirection.Rtl) { | ||
| return pageSets.length - 1 - idx | ||
| } else return idx | ||
| }, | ||
| [pageSets.length, readingDirection], | ||
| ) | ||
|
|
||
| /** | ||
| * A function that takes the slider value and returns the corresponding pageSet index | ||
| * It uses the same logic as getSliderValue | ||
| */ | ||
| const getPageSetIndex = useCallback((value: number) => getSliderValue(value), [getSliderValue]) |
There was a problem hiding this comment.
Super unimportant, but I would say these should be "inverted" so that getSliderValue invokes getPageSetIndex (basically just swapping the names I guess)
There was a problem hiding this comment.
Note: I didn't use the same function just for clarity. I could've used
const getPageSetIndex = getSliderValuebut then the input name being idx on both annoyed me. I've done the swap, so if I did that (inverted obv) they both would say value , which is okay, even though I only intended to use that word for slider value. Would that be better?
There was a problem hiding this comment.
No yeah I totally agree with your choice against just doing const getPageSetIndex = getSliderValue, this was just a super minor nit. I don't have a super strong opinion either way, just something that stuck out in my brain
| <Text className="text-center">{pageSet.map((i) => i + 1).join('-')}</Text> | ||
| <Text className="text-center"> | ||
| {pageSet | ||
| .sort((a, b) => a - b) // we always use (from left to right) the smaller then larger number even if using RTL (e.g. pages 3-4 and never 4-3) |
❤️ Aww I like those words
Well it might be okay then. When I tried removing the |
Haha I'm sure it's not the latter. Fwiw I do also remember some weirdness and flickering when using FlastList for the reader, in fact I recall writing a comment somewhere something to the effect of "this reader really doesn't like flashlist" which in hindsight is really vague. But also that was v1, I haven't tried again since upgrading. Also I'd be curious how much benefit using it would even provide. I'd love to measure it, if possible, but I imagine most of the benefit will come to those using Android devices. For whatever reason, react native often just feels more optimized on iOS unfortunately. For instance, I just loaded up my TPB Invincible Universe (660 pages) almost instantaneously with no stutter or lag. I'll aim to merge this shortly 🙂 |
2 participants
This contains various fixes for RTL mode, and of course make it work in the first place (it was a double inversion issue). I'm so happy to finally be able to read my books without the swipe direction confusing me.
It also contains a few visual improvements:
I saw some TODO's that said swap to
FlashListfor performance improvements, but they don't have anyinvertedprop (which the RTL logic currently relies on and I don't feel like changing it unless I have to). I also saw another comment somewhere else aboutLegendListand those do have aninvertedprop, so I might give that a go.I also updated my little list to be easy to tick completed stuff: #770 (comment)
I'm curious about if you look at each commit separately or the whole thing at once?