Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy loading for scrollable Tabs during mounting and rendering. #303

Closed
wants to merge 7 commits into from

Conversation

code-smith
Copy link
Contributor

Made code changes to mount only initial tab when loaded and lazily mount other tabs whenever selected. By default it mounts all tabs at once.
Once all tabs are mounted, only renders selected tab instead of all tabs.

…t tabs only when clicked for first time and render only selected tabs
@@ -18,6 +19,28 @@ const TimerMixin = require('react-timer-mixin');
const DefaultTabBar = require('./DefaultTabBar');
const ScrollableTabBar = require('./ScrollableTabBar');

class StaticContainer extends Component{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i dint know about this. Logic's no different. Let me know, i can change it if you insist

Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to paste this as well. There is an entire subproj: https://github.com/reactjs/react-static-container. Should use that instead of you own.

Copy link
Contributor Author

@code-smith code-smith Jun 10, 2016

Choose a reason for hiding this comment

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

@cancan101 I'll change it in the final submission. Appreciate if you can give feedback on other part of logic and test it's effectiveness .

Copy link
Contributor Author

@code-smith code-smith Jun 10, 2016

Choose a reason for hiding this comment

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

@cancan101 @skv-headless i made changes and used the external component mentioned, package.json had to be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cancan101 were you able to implement lazy rendering of selected tab without static container from outside library?

@skv-headless
Copy link
Collaborator

skv-headless commented Jun 10, 2016

I need some time to understand this. Please be patient. I thought about it. We can't solve everything for user. For ex

  1. Lazy should be optional (for me it is working good without laziness)
  2. What to show before tab loaded (loader, white view, some image)
  3. Use static container or not.

Also it is possible to do outside the library to control everything.

@code-smith
Copy link
Contributor Author

code-smith commented Jun 10, 2016

Let me chip in.
#1. Lazy mounting cannot be done outside the Library at all. Lazy mounting can only be done within the library.
Though it's good to give an option whether to lazy mount or not i don't think it would be of much help.
Mounting all tabs at once can really slow down your application and if it you application isn't heavy to slow down then it would definitely not slow down with lazy mount enabled.
So why give such option? And why mount all tabs initially?
I think its more of requirement now to lazy mount/load form performance point of view. I know i'm strongly opinionated here.
If lazy load option needs to be given then i can make some code changes. Though i still believe tabs should only be mounted lazily, for the component to be high performing.

#2. What to show before tab loading, is something thats very application specific and can/should be done outside the library for each tab. It's something user can/should do within his tab (child).

#3. Using Static container is a good practice that improves performance when there are multiple views. why re-render what's not selected when the user's intention is to view only what he is selecting?
Rendering all can get very expensive and its a very bad behaviour and i strongly believe static container should be implemented.
Mounting all children at once happens only once initially but rendering keeps happening every time user scrolls and its very very bad to render all children.

As far as features that can be done outside the library:
#1 Lazy Mounting cannot be controlled outside library, hence the motivation to change the code.
#2 Loaders and spinners etc can and should be done by user specific to his/her application requirement outside the library, I don't think we should worry about this.
#3 I was not able to render tabs only for selected tabs from outside the library, @skv-headless please give an example.

@@ -28,6 +28,7 @@
},
"homepage": "https://github.com/brentvatne/react-native-scrollable-tab-view#readme",
"dependencies": {
"react-static-container": "^1.0.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/facebook/react-native/blob/50c2467905815dbb434beecd0e649fb4a7bc2e20/Libraries/react-native/react-native.js
https://github.com/facebook/react-native/blob/50c2467905815dbb434beecd0e649fb4a7bc2e20/Libraries/Components/StaticContainer.js
Though it is shipped in the code, its not exported to the main ReactNative object.
It's not working like in the tabbarios, so had to do it from external component(i think its an offical release , not sure)
@skv-headless Please suggest alternative if not that plugin.I would like to wrap this whole thing today itself

@skv-headless
Copy link
Collaborator

Please fix eslint warnings

renderScrollableContent() {
if (Platform.OS === 'ios') {
let scenes=[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this code same for both platforms. Please create a function or component instead of duplicating code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

<SceneComponent
key={key}
style={{width: this.state.containerWidth, }}
selected={selected}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think selected is unnecessary variable, just write false here.

Copy link
Contributor Author

@code-smith code-smith Jun 12, 2016

Choose a reason for hiding this comment

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

it's needed in the _.keyExists case, where it's value is changed to true if its current page

@skv-headless
Copy link
Collaborator

Sorry that I'm so picky but it helps me to understand code

@code-smith
Copy link
Contributor Author

It's cool, You'd like to maintain consistency. Will make changes.


const StaticContainer = require('react-static-container');

class SceneComponent extends Component {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@code-smith
Copy link
Contributor Author

All changes done. Let me know

@oubushixb
Copy link

great work!

@@ -96,8 +105,26 @@ const ScrollableTabView = React.createClass({
}
},

updateSceneKeys(children, sceneKeys = [], currentPage) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the same as?

sceneKeys.concat([this._makeSceneKey(children[currentPage])])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.not even close.

Copy link
Collaborator

@skv-headless skv-headless Jun 15, 2016

Choose a reason for hiding this comment

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

Is this correct if sceneKeys would be a Set?
I'm looking to log and this function add a key of currentPage to sceneKeys
What is the difference?

Copy link
Contributor Author

@code-smith code-smith Jun 15, 2016

Choose a reason for hiding this comment

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

yes, distinct keys, which are added only on first time scroll/click.
Lets say out of 5 given tabs, user goes to tab2 for the first time from initial tab tab 1, and then tab3 for the first time ,now you have 3 keys .User doesnt open tab4/5 but goes back to tab2 again, then new key shouldnt be added, only 3 keys should be present. Only when user scrolls to tab4/5 for the first time keys for tab4/5 should be added. Once #keys === #children, its not updated anymore

if(this.props.children.length !== this.state.sceneKeys.length) {
let newKeys = this.updateSceneKeys(this.props.children, this.state.sceneKeys, pageNumber);
this.setState({currentPage: pageNumber, sceneKeys: newKeys, });
}else{
Copy link
Collaborator

Choose a reason for hiding this comment

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

eslint error. Could you please fix this

@skv-headless
Copy link
Collaborator

Could you please squash and rebase commits?

@code-smith
Copy link
Contributor Author

@skv-headless please be responsive, this is taking a lot of my time. mind sharing email or skype to finish this quickly. I'm spending way too much time on this

@skv-headless
Copy link
Collaborator

Your changes in master

@code-smith
Copy link
Contributor Author

Amazing!! Hope it helps others too :)

@skv-headless
Copy link
Collaborator

It will. Thanks

@code-smith
Copy link
Contributor Author

When can we have an npm release of this updated version?

@lalatgithub
Copy link

lalatgithub commented Apr 27, 2017

wasn't this merged and updated on npm? i really need this... having 2 tabs with network calls

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.

None yet

5 participants