Skip to content

Conversation

@ghuh
Copy link
Contributor

@ghuh ghuh commented Jul 25, 2016

When there is a change in tabs via props in ScrollableTabBar, recalculate width.

There can be display issues when the tabs are changed after the ScrollableTabBar is rendered. The issues can be either tabs piling on top of each other instead of scrolling, or the tabs scroll as if there are far more tabs than there currently are. Either way, recalculating the width solves the issue.

@skv-headless
Copy link
Collaborator

I hate to use componentWillReceiveProps. It usually leads to bugs. Can you please provide code snippet to reproduce that bug. I hope there is a better solution

@ghuh
Copy link
Contributor Author

ghuh commented Jul 28, 2016

This will trigger it. Note that you'll need a narrow screen. Any non-'Plus' iPhone in portrait should do it.

getInitialState: function()
{
    return {
        tabs: [ 'short', 'list' ],
    };
},
<ScrollableTabView
    renderTabBar = { () => <ScrollableTabBar /> }
>
        {
            this.state.tabs.map(
                (tab,index) =>
                    (
                    <View
                        tabLabel = { tab }
                        key = {index}
                    >
                        <TouchableHighlight
                            onPress = { () => this.setState({ tabs: [ 'short', 'list' ]})}
                        >
                            <Text> { 'short' } </Text>
                        </TouchableHighlight>
                        <TouchableHighlight
                            onPress = { () => this.setState({ tabs: [ 'long', 'list', 'with', 'very', 'many long tabs forcing', 'it to', 'scroll', 'and even longer' ]})}
                        >
                            <Text> { 'long' } </Text>
                        </TouchableHighlight>

                    </View>
                    )
            )
        }
</ScrollableTabView>

@ghuh
Copy link
Contributor Author

ghuh commented Aug 4, 2016

@skv-headless Any thoughts on another way to solve this problem?

@skv-headless
Copy link
Collaborator

Could you make showcase here https://github.com/skv-headless/react-native-scrollable-tab-view/blob/master/examples/FacebookTabsExample/DynamicExample.js so I will be able to check that it is working.
BTW thanks 👍 I wanted to fix this use case but forgot 😞

@ghuh
Copy link
Contributor Author

ghuh commented Aug 5, 2016

I added a new example called ChangingScrollableTabsExample.js. If you remove my fix to ScrollableTabBar, you can see the bug in action. Just click back and forth between "long" and "short".

@ghuh
Copy link
Contributor Author

ghuh commented Aug 13, 2016

@skv-headless Any thoughts now that you have a working test case?

@gkuhn1
Copy link

gkuhn1 commented Aug 20, 2016

+1
I have the same issue loading dynamic tabs from an API.
Fix by @ghuh make it work smoothly!
Waiting for the merge and a new release.

@ghuh
Copy link
Contributor Author

ghuh commented Sep 25, 2016

@skv-headless Can this be merged?

@skv-headless
Copy link
Collaborator

skv-headless commented Sep 25, 2016

I tried this patch. I've updated DynamicExample to use ScrollableTabBar. If you remove example I'll merge it. Please squash and rebase commits


componentWillReceiveProps(nextProps) {
// If the tabs change, force the width of the tabs container to be recalculated
if (JSON.stringify(this.props.tabs) != JSON.stringify(nextProps.tabs) && this.state._containerWidth) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you think that shallow comparison would be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I used JSON.stringify. It's a poor man's deep equals. There are more precise ways to do it but they all take a lot more code and have slower performance.

Regardless, yes, I believe this should be sufficient to ensure that any change will trigger the state to update correctly.

…and forth between needing to scroll and not needing to scroll"

This reverts commit a883cb8.
@ghuh
Copy link
Contributor Author

ghuh commented Sep 25, 2016

I reverted the example code and there are no conflicts with master so it should be good to merge.

@skv-headless skv-headless merged commit ef1877b into ptomasroos:master Sep 26, 2016
anchengjian pushed a commit to anchengjian/react-native-scrollable-tab-view that referenced this pull request Sep 29, 2018
A change in tabs can produce display issues.
caglaruba pushed a commit to caglaruba/react-native-scrollable-tab-view that referenced this pull request Aug 22, 2019
A change in tabs can produce display issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants