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

New Tabs component implementation #2061

Merged
merged 30 commits into from
Sep 17, 2024
Merged

Conversation

Mil4n0r
Copy link
Collaborator

@Mil4n0r Mil4n0r commented Jul 26, 2024

Checklist
(Check off all the items before submitting)

  • Build process is done without errors. All tests pass in the /lib directory.
  • Self-reviewed the code before submitting.
  • Meets accessibility standards.
  • Added/updated documentation to /website as needed.
  • Added/updated tests as needed.

Description
Current DxcTabs API is confusing, as it is expected to work in a similar way as DxcNavTabs, but it does not provide Compound Components, so we have no access to onClick/onHover events separately for each Tab.

For consistency purposes and easier implementation of the wrappers required in Halstack Studio and UXPin, it is convenient to redo it.

For now, we are maintaining the old API available to prevent adding breaking changes, however, those props will be marked as LEGACY/DEPRECATED to not encourage using them in the future (and eventually, removing them).

@Mil4n0r Mil4n0r changed the title New Tabs component implementation New Tabs component implementation Jul 26, 2024
@Mil4n0r Mil4n0r marked this pull request as ready for review July 29, 2024 11:11
@jsuarezgonz jsuarezgonz self-assigned this Aug 27, 2024
@jsuarezgonz jsuarezgonz removed their assignment Aug 30, 2024
@Jialecl Jialecl self-requested a review September 2, 2024 14:50
@Jialecl Jialecl self-assigned this Sep 2, 2024
@Jialecl Jialecl removed their request for review September 6, 2024 09:15
@Jialecl Jialecl removed their assignment Sep 6, 2024
@Mil4n0r
Copy link
Collaborator Author

Mil4n0r commented Sep 16, 2024

Good job @Jialecl. Changes look legit. The only thing to improve I can think of would be to improve the typings so that only legacy behavior or new behavior are allowed, not both at once. However, I am not sure if that is worth it taking into account that the legacy behavior will eventually be removed.

Let me know what you guys think @Jialecl @GomezIvann.

packages/lib/src/tabs/Tab.tsx Outdated Show resolved Hide resolved
packages/lib/src/tabs/Tabs.tsx Outdated Show resolved Hide resolved
packages/lib/src/tabs/Tabs.tsx Outdated Show resolved Hide resolved
packages/lib/src/tabs/Tabs.tsx Outdated Show resolved Hide resolved
packages/lib/src/tabs/types.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@GomezIvann GomezIvann left a comment

Choose a reason for hiding this comment

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

Just a general thought. There are too many variables depending on each other (because of useMemo) inside Tabs.tsx which are only use once or twice. For example:

  1. hasActiveChild, initialActiveTabLabel can be transformed into an initialization function inside the useState of activeTabLabel:
  const childrenArray: ReactElement<TabProps>[] = useMemo(
    () => Children.toArray(children) as ReactElement<TabProps>[],
    [children]
  );
  const hasLabelAndIcon = useMemo(
    () => childrenArray.some((child) => isValidElement(child) && child.props.icon && child.props.label),
    [childrenArray]
  );

  const [activeTabLabel, setActiveTabLabel] = useState(() => {
    const hasActiveChild = childrenArray.some(
      (child) => isValidElement(child) && (child.props.active || child.props.defaultActive) && !child.props.disabled
    );
    const initialActiveTab = hasActiveChild
      ? childrenArray.find(
          (child) => isValidElement(child) && (child.props.active || child.props.defaultActive) && !child.props.disabled
        )
      : childrenArray.find((child) => isValidElement(child) && !child.props.disabled);

    return isValidElement(initialActiveTab) ? initialActiveTab.props.label : "";
  });
  1. You can directly pass a function to aria-disabled from ActiveIndicator in order to remove another const (isActiveIndicatorDisabled).
                  aria-disabled={childrenArray.some((child) =>
                    isValidElement(child) ? activeTabLabel === child.props.label && child.props.disabled : false
                  )}

Since you are already memorizing the value of childrenArray, all this changes make sense and don't trigger rerenders.

@GomezIvann GomezIvann self-assigned this Sep 17, 2024
@GomezIvann GomezIvann merged commit 0cd0750 into master Sep 17, 2024
4 checks passed
@GomezIvann GomezIvann deleted the Mil4n0r/new_tabs_implementation branch September 17, 2024 11:53
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.

4 participants