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

refactor(v2): render all tab panels at once #3706

Merged
merged 2 commits into from
Nov 9, 2020

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Nov 6, 2020

Motivation

I think we need to render all tab panels immediately during mounting Tabs component, because currently different side effects observer when navigating between tabs.

For example, if the tab content contains embedded code, then it will reload every time we switch tabs, see the GIF on the React Native website example:

ezgif com-gif-maker (10)

In addition, filtering rendering (as now) leads to additional overhead costs, this is especially noticeable on mobile devices (you can see the delay when switching tabs)

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Nov 6, 2020
@lex111 lex111 requested a review from slorber as a code owner November 6, 2020 12:04
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 6, 2020
@netlify
Copy link

netlify bot commented Nov 6, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 47d91f6

https://deploy-preview-3706--docusaurus-2.netlify.app

@slorber
Copy link
Collaborator

slorber commented Nov 6, 2020

I don't think we should always render eagerly each tab.

In the case of RN, each snack code block is an iframe, loading eagerly iframes can be expensive, particularly when we are likely to only show one, as most users will not switch tabs, so I'd rather keep the lazy behavior even if it requires more integration code (cc @Simek)

But an option to customize this behavior could be useful. Rendering eagerly would also permit to have all tabs be SEO indexed more easily.

Which default value to use? Should we prioritize performance or SEO?

React-navigation tabs are lazy by default: https://reactnavigation.org/docs/bottom-tab-navigator/#lazy
But they do not care about SEO

@lex111
Copy link
Contributor Author

lex111 commented Nov 6, 2020

I don't think performance will suffer much in that case. We can check this, bu tf this was not a big issue in old React Native website which powered on Docusaurus v1 https://docusaurus.io/docs/en/doc-markdown#language-specific-code-tabs, then everything will be OK in v2 too.

Many UI libraries renders all tab panels at once, for example:

Rendering eagerly would also permit to have all tabs be SEO indexed more easily.

Oh yeah, I didn't think about it initially, but it will be good for SEO!

React-navigation tabs are lazy by default: https://reactnavigation.org/docs/bottom-tab-navigator/#lazy

It's different since it is React Native so makes sense to use lazy loading on mobiles.

@slorber
Copy link
Collaborator

slorber commented Nov 6, 2020

In v1 I don't know if they had much tabs. Wasn't able to find any in RN 0.59.

Let's compare before/after on a page with 2 snack examples

With eager behavior, the 2nd would render 4 iframes
With lazy behavior, the 2nd would render 2 iframes

I'd rather have only 2 iframes, but maybe 4 wouldn't be a problem.

It's also possible to add a loading="lazy" attributes on the Snack iframes: https://web.dev/iframe-lazy-loading/

Wonder what @Simek and @IjzerenHein (working on Snack) think about this, but I guess too many iframes loaded could be a problem, and the RN website is likely to add more examples of this kind over time.

I think we should keep an option for lazy loading tabs in all cases, and changing the default behavior can be considered a breaking change. Snacks wouldn't benefit from SEO/indexing in any way and lazy loading is better in such case.

@lex111
Copy link
Contributor Author

lex111 commented Nov 6, 2020

This is a rather specific use case, in most cases we are dealing with static content in tabs, which would be nice to be indexed by search robots by default. But originally I was worried about how tabs were rendered on mobile devices. On not the most modern/powerful mobiles there is appreciable delay in navigating between tabs, so I would like to avoid it.

I don't mind adding control of this behavior via new prop, for example, but if we can lazily loading iframes, then maybe we shouldn't do it?

@slorber
Copy link
Collaborator

slorber commented Nov 9, 2020

I think having a lazy boolean prop would still be helpful in some cases.

Also it's a behavior change, if users expected the tabs to be lazy, maybe making them eager would break their site for some reason, and we'd rather provide an option to allow previous behavior.

loading=lazy is not widely supported yet and give less control to the user, and will not apply in all situations

@lex111
Copy link
Contributor Author

lex111 commented Nov 9, 2020

Done, feel free to edit the docs if you see fit.

</div>

{lazy ? (
cloneElement(
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't the div wrapper missing here?

Copy link
Contributor Author

@lex111 lex111 Nov 9, 2020

Choose a reason for hiding this comment

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

We don't need div wrapper if we render only single tab panel. If multiple tab panels are rendered (!lazy), then they will be in the div wrapper. This is a kind of optimization so as not to create extra div elements and follow ARIA meanwhile.

Copy link
Collaborator

@slorber slorber Nov 9, 2020

Choose a reason for hiding this comment

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

Ah I see, didn't notice that you moved the className to the cloned element, thought that spacing was lost

@slorber
Copy link
Collaborator

slorber commented Nov 9, 2020

FYI @Simek, on next release the snack player remark plugin should maybe use this new lazy attribute to keep existing behavior.

@slorber slorber merged commit 8c9f948 into master Nov 9, 2020
@IjzerenHein
Copy link

@slorber I don't have a strong preference in this, but loading less always seems like a good idea. If possible, lazy loading the Snack iFrames seems smart. I would try to prevent to load too many Snack iFrames at the same time if possible though, and be mindful of the CPU and network usage.

As for Snack loading times. We are just in the process of rolling out some performance improvements. After this, consecutive loads of the web-preview should be snappier and close to near instant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants