feat(v2): auto focus to tab if it is outside viewport #4209

lex111 · 2021-02-09T17:23:13Z

Motivation

Trying to fix #3728

Currently, the thing is if tab sync is enabled, and going to switch from long-content tab to short one, we will not see a new tab on the screen. This is confusing for users and they have to manually scroll to the desired tab.

I suggest avoiding this unpleasant action by automatically focusing on the tab, but only if really necessary (when the new tab is outside the viewport).

I intentionally didn't do this via the new prop because it's a useful UX improvement that anyone using the synching tabs feature will need.

To achieve this, we had to move hideable navbar state to the context provider (by analogy with the announcement bar).

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Before	After

Used code for test

import Tabs from '@theme/Tabs';
import TabItem from '@theme/TabItem';

<Tabs
  groupId="test"
  defaultValue="one"
  values={[
    { label: 'Long tab', value: 'one' },
    { label: 'Short tab', value: 'two' }
]}>
<TabItem value="one">

<div style={{height: '1000px', border: '1px solid'}}></div>

</TabItem>

<TabItem value="two">

<div style={{height: '100px', border: '1px solid'}}></div>

</TabItem>
</Tabs>

---

<Tabs
  groupId="test"
  defaultValue="one"
  values={[
    { label: 'Long tab', value: 'one' },
    { label: 'Short tab', value: 'two' }
]}>
<TabItem value="one">

<div style={{height: '1000px', border: '1px solid'}}></div>

</TabItem>

<TabItem value="two">

<div style={{height: '100px', border: '1px solid'}}></div>

</TabItem>
</Tabs>

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 · 2021-02-09T17:24:33Z

packages/docusaurus-theme-common/src/utils/dom.ts

+  return top >= 0 && right <= innerWidth && bottom <= innerHeight && left >= 0;
+}
+
+export const domUtils = new Proxy(


I decided to use proxy to simplify checking the DOM environment (otherwise it was necessary to include duplicated check condition in each util function).
Is this an acceptable solution or should we use the "traditional" option?

I'd prefer the traditional option, Proxy complicates things for little value and will also mess-up with the TypeScript support that we want to improve in the long term.

netlify · 2021-02-09T17:28:19Z

[V1] Deploy preview success

Built with commit b9583f3

https://deploy-preview-4209--docusaurus-1.netlify.app

github-actions · 2021-02-09T17:31:56Z

Size Change: +196 B (0%)

Total Size: 532 kB

Filename	Size	Change
`website/build/assets/css/styles.********.css`	87.3 kB	+196 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`website/build/assets/js/main.********.js`	359 kB	0 B
`website/build/blog/2017/12/14/introducing-docusaurus/index.html`	60.3 kB	0 B
`website/build/docs/introduction/index.html`	235 B	0 B
`website/build/index.html`	25.4 kB	0 B

_{compressed-size-action}

netlify · 2021-02-09T17:33:46Z

Deploy preview for docusaurus-2 ready!

Built with commit b9583f3

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

github-actions · 2021-02-09T17:33:59Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	83
🟢 Accessibility	96
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	95

Lighthouse ran on https://deploy-preview-4209--docusaurus-2.netlify.app/classic/

lex111 · 2021-02-18T05:01:22Z

@slorber please take a look at this PR. I would like to see it in new release.

slorber

I think the solution and code added to solve the problem is too complex and suggested a simpler solution.

In my opinion the current implementation of Tabs is not very good, not idiomatic React, and should be refactored totally in the future. I'd preferably like to avoid adding too much complexity to tabs so that a future refactoring would be simpler to achieve, and so that we can invest in the ideal solution instead of a good enough one

slorber · 2021-02-18T10:51:38Z

packages/docusaurus-theme-common/src/utils/dom.ts

+  return top >= 0 && right <= innerWidth && bottom <= innerHeight && left >= 0;
+}
+
+export const domUtils = new Proxy(


I'd prefer the traditional option, Proxy complicates things for little value and will also mess-up with the TypeScript support that we want to improve in the long term.

slorber · 2021-02-18T11:20:33Z

packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx

+            window.scrollBy(0, -navbarHeight);
+          }
+        }, 50);
+      }, 150);


In an ideal world we'd want the mouse cursor to stay above the tab we just clicked without the user seeing things scroll (using useLayoutEffect), but this is more complex to achieve.

As your solution is not the ideal solution, but more a "best effort, good enough", and I think it's a bit too complicated for little value.

Something like this is simpler and even better in my opinion:

if (groupId != null) { setTabGroupChoices(groupId, selectedTabValue); setTimeout(() => { if (domUtils.isInViewport(selectedTab)) { return; } selectedTab.scrollIntoView({ block: 'center', behavior: 'smooth', }); }, 150); }

block start is not the best for me because the user will click on a tab in the middle of the screen and that tab will appear at the very top of the screen afterward. Without animation, he may struggle to understand what is happening. Centering the tab makes more sense to me and handle the collapsible scrollbar at the same time with less complexity and much less code to ship/maintain

Alright, so I also added a short blicker on the active tab item to point the user to the correct tab. Or do you think this is superfluous?

Changed behavior:

I have not reverted the other code. For example, managing the state of the hideable navbar is useful to us to fix a bug with docsearch (after closing the search modal, the navbar is closed for some reason).

slorber · 2021-02-19T18:50:21Z

packages/docusaurus-theme-classic/src/theme/hooks/useHideableNavbar.ts

+      domUtils.convertRemToPx(
+        domUtils.getPropertyValue('--ifm-navbar-height'),
+      )) ||
+    0;


Does it give a different height compared to before?
I feel this solution is more complex, more likely to break (for example if we change the rem unit to something else) and more expensive to run.

Isn't reading the stylesheet in render expensive?

As understand, these functions have about the same effect on performance, but I agree with you, so I simplified the solution as you suggested above.

lex111 · 2021-03-01T00:41:24Z

packages/docusaurus-theme-classic/src/theme/Tabs/styles.module.css

+  animation: blink 0.5s ease-in-out 5;
+}
+
+@keyframes blink {


Not sure that this animation looks good, so it might be worth removing.

slorber · 2021-03-04T16:32:23Z

LGTM, just fixed a typo add()->remove()

lex111 added the pr: new feature This PR adds a new API or behavior. label Feb 9, 2021

lex111 requested a review from slorber as a code owner February 9, 2021 17:23

facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 9, 2021

lex111 commented Feb 9, 2021

View reviewed changes

slorber requested changes Feb 18, 2021

View reviewed changes

lex111 force-pushed the lex111/iss3728 branch 2 times, most recently from dfd4538 to 05d9947 Compare February 18, 2021 15:57

slorber reviewed Feb 19, 2021

View reviewed changes

Simplify solution

06abc2f

lex111 force-pushed the lex111/iss3728 branch from 4252a17 to 06abc2f Compare March 1, 2021 00:35

lex111 commented Mar 1, 2021

View reviewed changes

lex111 requested a review from slorber March 1, 2021 01:00

lex111 added this to the v2.0.0-alpha.71 milestone Mar 4, 2021

fix typo

b9583f3

slorber merged commit 788b4a7 into master Mar 4, 2021

slorber deleted the lex111/iss3728 branch August 17, 2021 17:50

Shrugsy mentioned this pull request Sep 26, 2021

feat: maintain page position for clicked grouped tabs #5618

Merged

Simek mentioned this pull request Oct 7, 2021

unwanted website scroll to top on refocus facebook/react-native-website#2807

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(v2): auto focus to tab if it is outside viewport #4209

feat(v2): auto focus to tab if it is outside viewport #4209

lex111 commented Feb 9, 2021

lex111 Feb 9, 2021 •

edited

Loading

slorber Feb 18, 2021

netlify bot commented Feb 9, 2021 •

edited

Loading

github-actions bot commented Feb 9, 2021 •

edited

Loading

netlify bot commented Feb 9, 2021 •

edited

Loading

github-actions bot commented Feb 9, 2021 •

edited

Loading

lex111 commented Feb 18, 2021

slorber left a comment

slorber Feb 18, 2021

slorber Feb 18, 2021

lex111 Feb 18, 2021

slorber Feb 19, 2021

lex111 Mar 1, 2021

lex111 Mar 1, 2021

slorber commented Mar 4, 2021

feat(v2): auto focus to tab if it is outside viewport #4209

feat(v2): auto focus to tab if it is outside viewport #4209

Conversation

lex111 commented Feb 9, 2021

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

lex111 Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

slorber Feb 18, 2021

Choose a reason for hiding this comment

netlify bot commented Feb 9, 2021 • edited Loading

github-actions bot commented Feb 9, 2021 • edited Loading

netlify bot commented Feb 9, 2021 • edited Loading

github-actions bot commented Feb 9, 2021 • edited Loading

lex111 commented Feb 18, 2021

slorber left a comment

Choose a reason for hiding this comment

slorber Feb 18, 2021

Choose a reason for hiding this comment

slorber Feb 18, 2021

Choose a reason for hiding this comment

lex111 Feb 18, 2021

Choose a reason for hiding this comment

slorber Feb 19, 2021

Choose a reason for hiding this comment

lex111 Mar 1, 2021

Choose a reason for hiding this comment

lex111 Mar 1, 2021

Choose a reason for hiding this comment

slorber commented Mar 4, 2021

lex111 Feb 9, 2021 •

edited

Loading

netlify bot commented Feb 9, 2021 •

edited

Loading

github-actions bot commented Feb 9, 2021 •

edited

Loading

netlify bot commented Feb 9, 2021 •

edited

Loading

github-actions bot commented Feb 9, 2021 •

edited

Loading