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

[Tabs] Fix RTL indicator #26470

Merged
merged 13 commits into from Jun 2, 2021
Merged

[Tabs] Fix RTL indicator #26470

merged 13 commits into from Jun 2, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented May 27, 2021

closes #26450

From the open issue, rtl does not work well with the current implementation because it force css left so the indicator start from the left side which makes it weird.

Preview: https://deploy-preview-26470--material-ui.netlify.app/components/tabs/

@siriwatknp siriwatknp added the component: tabs This is the name of the generic UI component, not the React module! label May 27, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented May 27, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against a743a44

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Could we add a reproduction on #26450?

docs/src/pages/components/tabs/tabs.md Outdated Show resolved Hide resolved
docs/src/pages/components/tabs/tabs.md Outdated Show resolved Hide resolved
packages/material-ui/src/Tabs/Tabs.js Outdated Show resolved Hide resolved
@siriwatknp
Copy link
Member Author

Could we add a reproduction on #26450?

able to reproduce but the fix makes the code more complicated. Not really sure if we want to fix this edge case or not given rtl is minor @oliviertassinari

@siriwatknp
Copy link
Member Author

One improvement that I see is this case. I was trying to use Tabs as navigation in some project and it does not have initial value at first, when hover on the menu (tab) it show underline indicator but because of initial indicator style, it always stretch from the left which looks weird to me. It would be nice if value change from false to number and the indicator grow from the middle to the hovered Tab.

May-28-2564.17-17-30.mp4

@siriwatknp siriwatknp requested a review from mnajdova May 28, 2021 10:24
@oliviertassinari oliviertassinari changed the title [Tabs] fix RTL indicator [Tabs] Fix RTL indicator May 28, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Almost good to ship

docs/src/pages/components/tabs/HoverTabs.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/tabs/HoverTabs.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/tabs/HoverTabs.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/tabs/HoverTabs.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/tabs/tabs.md Outdated Show resolved Hide resolved
packages/material-ui/src/Tabs/Tabs.js Outdated Show resolved Hide resolved
packages/material-ui/src/Tabs/Tabs.test.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label May 28, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented May 28, 2021

Not really sure if we want to fix this edge case or not given rtl is minor

@siriwatknp Yeah, we might not have worked on this problem, especially not now where we need to ship v5, but no big deal, it's not a big effort, half a day work, top. I think that we could even leave the issue count to grow to 1,000/3,000 while cleaning them up along the way.

the indicator grow from the middle to the hovered Tab.

Maybe it should start from the first tab? Something in this order:

diff --git a/packages/material-ui/src/Tabs/Tabs.js b/packages/material-ui/src/Tabs/Tabs.js
index 68273478c1..81660ada37 100644
--- a/packages/material-ui/src/Tabs/Tabs.js
+++ b/packages/material-ui/src/Tabs/Tabs.js
@@ -310,6 +310,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
   const getTabsMeta = () => {
     const tabsNode = tabsRef.current;
     let tabsMeta;
+    let firstTabsMeta;
     if (tabsNode) {
       const rect = tabsNode.getBoundingClientRect();
       // create a new object with ClientRect class props + scrollLeft
@@ -327,44 +328,52 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
     }

     let tabMeta;
-    if (tabsNode && value !== false) {
+    if (tabsNode) {
       const children = tabListRef.current.children;

       if (children.length > 0) {
-        const tab = children[valueToIndex.get(value)];
-        if (process.env.NODE_ENV !== 'production') {
-          if (!tab) {
-            console.error(
-              [
-                `Material-UI: The value provided to the Tabs component is invalid.`,
-                `None of the Tabs' children match with \`${value}\`.`,
-                valueToIndex.keys
-                  ? `You can provide one of the following values: ${Array.from(
-                      valueToIndex.keys(),
-                    ).join(', ')}.`
-                  : null,
-              ].join('\n'),
-            );
+        if (value !== false) {
+          const tab = children[valueToIndex.get(value)];
+          if (process.env.NODE_ENV !== 'production') {
+            if (!tab) {
+              console.error(
+                [
+                  `Material-UI: The value provided to the Tabs component is invalid.`,
+                  `None of the Tabs' children match with \`${value}\`.`,
+                  valueToIndex.keys
+                    ? `You can provide one of the following values: ${Array.from(
+                        valueToIndex.keys(),
+                      ).join(', ')}.`
+                    : null,
+                ].join('\n'),
+              );
+            }
           }
+          tabMeta = tab ? tab.getBoundingClientRect() : null;
+        } else {
+          const tab = children[0];
+          firstTabsMeta = tab ? tab.getBoundingClientRect() : null;
         }
-        tabMeta = tab ? tab.getBoundingClientRect() : null;
       }
     }
-    return { tabsMeta, tabMeta };
+    console.log({ tabMeta, firstTabsMeta })
+    return { tabsMeta, tabMeta, firstTabsMeta };
   };

   const updateIndicatorState = useEventCallback(() => {
-    const { tabsMeta, tabMeta } = getTabsMeta();
+    const { tabsMeta, tabMeta, firstTabsMeta } = getTabsMeta();
     let startValue = 0;

-    if (tabMeta && tabsMeta) {
+    const finalTabMeta = tabMeta || firstTabsMeta;
+
+    if (finalTabMeta && tabsMeta) {
       if (vertical) {
-        startValue = tabMeta.top - tabsMeta.top + tabsMeta.scrollTop;
+        startValue = finalTabMeta.top - tabsMeta.top + tabsMeta.scrollTop;
       } else {
         const correction = isRtl
           ? tabsMeta.scrollLeftNormalized + tabsMeta.clientWidth - tabsMeta.scrollWidth
           : tabsMeta.scrollLeft;
-        startValue = tabMeta.left - tabsMeta.left + correction;
+        startValue = finalTabMeta.left - tabsMeta.left + correction;
       }
     }

@siriwatknp
Copy link
Member Author

Maybe it should start from the first tab? Something in this order:

I think it is easier to use CSS if the user want to have this behaviour. Ideally, I think it should grow from middle of the tab index that I hover on but it is an improvement that we can do later. We can go with only rtl fix in this PR. thanks for your suggestion.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

25dd9ba it works :)

I'm not sure that this demo is accessible (that we want to include it) https://deploy-preview-26470--material-ui.netlify.app/components/tabs/#hover-tabs, cc @eps1lon for a point of view.

Otherwise, it looks solid 👍

@siriwatknp siriwatknp requested a review from eps1lon June 1, 2021 07:19
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Looks good. Just some minor test changes that should be resolved even though they are minor.

packages/material-ui/src/Tabs/Tabs.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/Tabs/Tabs.test.js Outdated Show resolved Hide resolved
siriwatknp and others added 3 commits June 2, 2021 08:05
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
@siriwatknp siriwatknp merged commit 34aa839 into mui:next Jun 2, 2021
@siriwatknp siriwatknp deleted the fix/tabs-rtl branch June 2, 2021 02:55
@oliviertassinari
Copy link
Member

A step forward 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tab indicator Animates from the left on RTL
4 participants