fix(Tab, TabView): conditional rendering #3397
Conversation
Codecov Report
@@ Coverage Diff @@
## next #3397 +/- ##
==========================================
+ Coverage 78.28% 78.41% +0.12%
==========================================
Files 87 87
Lines 1750 1751 +1
Branches 772 772
==========================================
+ Hits 1370 1373 +3
+ Misses 375 373 -2
Partials 5 5
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| } | ||
| const position = dx / -window.width; | ||
| const next = position > value ? Math.ceil(position) : Math.floor(position); | ||
| onChange?.(currentIndex.current + next); |
There was a problem hiding this comment.
For some reason this logic (75-77) broke with conditional children. I fixed by simply adding or subtracting 1 from the index based on the sign of dx
There was a problem hiding this comment.
After a second look, the original code was setting "value" only once upon initial render when panResponder was initialized with useRef. Since value was, by default, set to 0 - this code was effectively doing the same thing as my fix. This code could have caused bugs in its current state - for example, if the developer mounted the component with a starting value for "value" of a number greater than 0, it could have prevented the index from being incremented since position would never be greater than value. The reason it broke with dynamic children was because "value" was permitted to update whenever the dependency on length was added.
Would it be worthwhile to consider added a minimum dx threshold to change TabViews? Right now, as long as Abs(dx) is greater than Abs(dy), even if it's a very small number, a pan will always initiate the view change. Is that desirable? Or should we be checking if the pan is more than, say, 25% of the screen width?
There was a problem hiding this comment.
I am making changes here #3380. Feel free to review it.
There was a problem hiding this comment.
Looks great - exactly what I was thinking of. It looks like to implement my fix in your new code, React.Children.count(children) on line 76 will need changed to React.Children.toArray(children).length, and that same value will need to be included in the dependency array on 101 as well as a dependency for panResponder which means converting it from useRef to useMemo.
Nice work.
| return; | ||
| } | ||
|
|
||
| onChange?.(currentIndex.current + (dx > 0 ? -1 : 1)); |
There was a problem hiding this comment.
This works just as well as the previous logic (as tested in the example app) and is able to support conditional rendering.
| return; | ||
| useEffect(() => { | ||
| if (currentIndex.current > length - 1) { | ||
| onChange(length - 1); |
There was a problem hiding this comment.
This keeps the user from getting stuck beyond the last view if length changes
|
@WVAviator Can you merge latest |
…-native-elements into conditional-tabs
All set - tested again as well. Decided to omit the useEffect statement I had created before that kept the user from getting stuck past the last TabView - the index really needs to be managed in the parent component if/when the number of Views changes so I decided it wouldn't be a good idea to hardcode that behavior. |
|
Thank you for your contribution 🎉 |
Motivation
Fixes #3390
Type of change
How Has This Been Tested?
exampleappChecklist
Additional context
Switching to the use of React.Children.toArray() instead of React.Children.map() effectively ignores falsey children rather than throwing an error. However, assuming that a developer may want the ability to enable/disable tabs while the component is mounted created a couple other issues to solve: