COM-60: Additional features for Tabs #1392

jomunker · 2023-11-13T08:25:00Z

This PR adds new Features to the Tabs and RouterTabs Components. (Router-)Tabs can now contain the Divider component as children to separate Tabs. Additionally, one Tab or RouterTab can now show a tab icon, a status with icon and a tooltip.

Acceptance criteria

Add changeset

Screenshots

Implementation

Tabs

RouterTabs

… type

…mobile - add flags to tabs component - add disabled styling to TabScrollButton

… to router tabs

…mponent with extensions

…abs component

…om tab

…-features

lllHuber · 2023-11-16T09:30:21Z

@jomunker, @thomasdax98, @johnnyomair what's the status here? I would like to use this feature in a project and could help out, if something still needs to be done.

thomasdax98 · 2023-11-16T12:01:01Z

@jomunker Can you please rebase this branch and fix the conflicts?

packages/admin/admin/src/statusBadge/StatusBadge.tsx

…-features

johnnyomair · 2023-11-30T13:26:17Z

This PR has commits from #1351 -> maybe you need a rebase? 🤔

johnnyomair

The icons are also added in #1462, right?

johnnyomair · 2023-11-30T13:27:09Z

.changeset/polite-suits-turn.md

+"@comet/admin": minor
+---
+
+Add new features to `Tabs`. A tab can now have an icon, status and tooltip. A `Divider` component can be added alongside `Tab`/ `RouterTab` components in `Tabs` and `RouterTabs`.


@thomasdax98 is this changeset enough?

@jomunker Please add a code example showing how the Tabs can be configured to the changeset and/or a link to the storybook story showcasing the new functionality

Will do it once all discussions are resolved for this PR 👍🏻

packages/admin/admin/src/hooks/useCurrentDeviceType.ts

packages/admin/admin/src/statusBadge/StatusBadge.styles.ts

johnnyomair · 2023-11-30T13:29:45Z

packages/admin/admin/src/statusBadge/StatusBadge.tsx

+    showStatusIcon?: boolean;
+    statusIcon?: React.FunctionComponentElement<IconProps> | undefined;


Both these props should be controlled via status. Why do you need them?

I agree that we can remove showStatusIcon but the other one is needed imo. If the user wants a custom icon then it's needed. I mean we could use one status prop but as an object with type and icon

There are two versions of the status: with and without icon. We need to support both.

My opinion: Keep showStatusIcon and omit statusIcon. We have three predefined statuses with three predefined colors so we can also have three predefined icons.

Otherwise we have to make the whole component more flexible. But here I'm more pro ease-of-use vs flexibility.

johnnyomair · 2023-11-30T13:30:26Z

packages/admin/admin/src/statusBadge/StatusBadge.tsx

+    if (!status && (showStatusIcon || statusIcon)) console.warn("A status (prop: status) has to be provided, if the status icon should show.");
+
+    if (status && statusIcon && !showStatusIcon) console.warn("The status icon will only be shown, if the showStatusIcon prop is set to true.");
+


I don't think we need this warning if we remove showStatusIcon and statusIcon. If we need them, I'd rather throw an error

packages/admin/admin/src/tabs/Tabs.tsx

packages/admin/admin/src/tabs/CustomTab.tsx

johnnyomair · 2023-11-30T13:46:20Z

packages/admin/admin-stories/src/docs/components/Tabs/stories/Tabs.stories.tsx

+                    showTooltip
+                    tooltipMessage="Tooltip message"
+                    tooltipIcon={<HelpOutline fontSize="large" />}


Instead of these three props, have you thought about <Tab tooltip={<Tooltip><Icon/></Tooltip/>} />? Way more flexible IMO

Status could also be done via <Tab status={<StatusBadge />} />. This would prevent having to prop drill the StatusBadge, Tooltip Props and creating tight coupling between components that have only few things in common (one is the child of another)

Good idea. But then the user has to know these two components. Don't know if this is nice UX 🤔

packages/admin/admin-stories/src/docs/components/Tabs/stories/Tabs.stories.tsx

packages/admin/admin/src/tabs/CustomTab.tsx

jomunker · 2023-12-21T16:35:55Z

This PR has commits from #1351 -> maybe you need a rebase? 🤔

Rebase doesn't work since the tab auto scroll PR has been squashed 🤷🏻‍♂️ Should I just leave it like this? Won't change anything I guess and we're squashing here anyways...

jomunker · 2023-12-21T16:36:16Z

The icons are also added in #1462, right?

Nope

packages/admin/admin/src/hooks/useCurrentDeviceType.ts

thomasdax98 · 2023-12-27T09:37:08Z

packages/admin/admin/src/tabs/Tabs.tsx

    defaultIndex?: number;
    tabsState?: ITabsState;
+    smallTabText?: boolean;


+1 for size

thomasdax98 · 2023-12-27T09:48:00Z

packages/admin/admin/src/tabs/CustomDivider.tsx

+
+export const Divider: React.FC<DividerProps> = () => null;
+
+export const CustomDivider: React.FC<DividerProps> = ({ children, ...props }) => {


thomasdax98 · 2023-12-27T09:49:04Z

packages/admin/admin/src/tabs/CustomDivider.tsx

+
+export const Divider: React.FC<DividerProps> = () => null;
+
+export const CustomDivider: React.FC<DividerProps> = ({ children, ...props }) => {


Although I think this was originally @jamesricky's suggestion. Maybe he can elaborate on that?

thomasdax98 · 2023-12-27T09:54:25Z

packages/admin/admin/src/tabs/CustomDivider.tsx

+
+export type DividerProps = Omit<MuiDividerProps, "orientation" | "variant">;
+
+export const Divider: React.FC<DividerProps> = () => null;


This is the same pattern we also use for Tab and RouterTab. But it isn't necessary for the Divider since we don't enhance it with more information later. So you could use CustomDivider directly in the application.

thomasdax98 · 2023-12-27T09:57:10Z

packages/admin/admin/src/tabs/CustomDivider.tsx

+
+export const Divider: React.FC<DividerProps> = () => null;
+
+export const CustomDivider: React.FC<DividerProps> = ({ children, ...props }) => {


If we decide to keep this component, I think it should be renamed to TabDivider.

packages/admin/admin/src/statusBadge/StatusBadge.tsx

thomasdax98 · 2023-12-27T10:19:49Z

packages/admin/admin/src/statusBadge/StatusBadge.tsx

+    showStatusIcon?: boolean;
+    statusIcon?: React.FunctionComponentElement<IconProps> | undefined;


There are two versions of the status: with and without icon. We need to support both.

My opinion: Keep showStatusIcon and omit statusIcon. We have three predefined statuses with three predefined colors so we can also have three predefined icons.

Otherwise we have to make the whole component more flexible. But here I'm more pro ease-of-use vs flexibility.

packages/admin/admin/src/tabs/RouterTabs.tsx

thomasdax98 · 2023-12-27T10:37:23Z

packages/admin/admin/src/tabs/Tabs.tsx

-    children: Array<React.ReactElement<TabProps> | boolean | null | undefined> | React.ReactElement<TabProps>;
-    tabComponent?: React.ComponentType<MuiTabProps>;
+    children: Array<React.ReactElement<TabProps | DividerProps> | boolean | null | undefined>;
+    tabComponent?: React.ComponentType<TabProps>;


Could this change from MuiTabProps to TabProps cause any compatibility problems?

The only thing would be icon and tabIcon I guess. Since @johnnyomair also stated this aswell, I changed it back to icon.

thomasdax98 · 2023-12-27T10:46:29Z

packages/admin/admin/src/tabs/Tab.tsx

+    tabIcon?: React.FunctionComponentElement<IconProps>;
+    status?: Status;
+    showStatusIcon?: boolean;
+    statusIcon?: React.FunctionComponentElement<IconProps>;
+    showTooltip?: boolean;
+    tooltipMessage?: string;
+    tooltipIcon?: React.FunctionComponentElement<IconProps>;
+    tooltipPlacement?: TooltipProps["placement"];
+    smallTabText?: boolean;


Regarding the props, I'd stay with the terms UX uses:

So
tabIcon -> icon
tooltipIcon -> helperIcon

I'd further:
remove showTooltip (should be controlled by helperIcon)
omit tooltipMessage and tooltipPlacement in favor of passing a Tooltip via helperIcon (Alternative: only pass helperText. Omit helperIcon, always show the Info icon instead)
rename smallTabText to size

Alright. @johnnyomair does this align with your expectations?

Should size be a number then or an enum value or what? Because at the moment it's a boolean and the name of a boolean variable (or prop in this case) should indicate that it is a boolean.

Co-authored-by: Thomas Dax <thomas.dax@vivid-planet.com>

…ibility issues

johnnyomair · 2024-01-10T15:53:27Z

We discussed this in today's meeting: There will be some UX changes (for instance, no tooltip), which need to be addressed in this PR before merging. Marking as draft for now.

johnnyomair · 2025-01-21T10:37:30Z

This PR hasn't been updated in over a year, so I'm closing it for now. If it's still relevant, feel free to update and re-open.

jomunker added 17 commits October 27, 2023 14:08

feat(useCurrentDeviceType): add hook which returns the current device…

95d39d2

… type

feat(Tabs): auto scroll buttons on desktop and scroll via swiping on …

2f09069

…mobile - add flags to tabs component - add disabled styling to TabScrollButton

feat(RouterTabs): add auto scrolling functionality and scroll buttons…

3a396e1

… to router tabs

feat(CustomTab): extract Tab into own file and create a custom tab co…

c7cb950

…mponent with extensions

feat(CustomTab): add flag to support small tab text

08fbc24

feat(CustomDivider): add custom divider and support for dividers in t…

a00e8aa

…abs component

feat(CustomTab): extract tab styles into classes

e4d05fd

feat(CustomTab): change disabled style

4e8cdd2

feat(CustomTab): throw error if the default tab was marked as disabled

9d76706

feat(RouterTabs): add support for divider component, make use of cust…

66aae57

…om tab

refactor(RouterTabs): rename props interface

a5b7785

feat(Tabs): add changeset

e445c9e

chore(admin): add @comet/admin-theme package

02536d5

feat(admin-theme): export warning palette

3f61b06

feat(CustomTab): add support for different status colors and icons

9a73618

feat(CustomTab): enforce status icon size

7a35084

feat(CustomTab): add example usage to RouterTabs and Tabs story

1311514

jomunker requested review from thomasdax98 and johnnyomair November 13, 2023 08:25

jomunker self-assigned this Nov 13, 2023

jomunker and others added 5 commits November 13, 2023 10:24

Merge remote-tracking branch 'origin/main' into COM-60-tab-additional…

e8ab252

…-features

chore: update pnpm-lock.yaml

61ef024

feat(icons): add new status icons

bc501ae

feat(CustomTab): use new status icons

08b26b4

chore(pnpm): regenerate pnpm-lock.yaml

7823c14

jomunker added 2 commits November 20, 2023 08:39

Merge branch 'main' into COM-60-tab-additional-features

3dbedf5

feat(StatusBadge): extract status badge into own comet admin component

59c4bcc

thomasdax98 reviewed Nov 21, 2023

View reviewed changes

packages/admin/admin/src/statusBadge/StatusBadge.tsx Outdated Show resolved Hide resolved

jomunker added 2 commits November 29, 2023 16:33

feat(StatusBadge): use theme instead of admin theme package

7263ffd

Merge remote-tracking branch 'origin/main' into COM-60-tab-additional…

b377878

…-features

jomunker requested a review from thomasdax98 November 30, 2023 09:49

johnnyomair reviewed Nov 30, 2023

View reviewed changes

Merge branch 'main' into COM-60-tab-additional-features

38ad42b

jomunker added 2 commits December 21, 2023 17:37

refactor(admin-icons): rename newly added icons to match convention

e031102

feat(admin-admin): remove useCurrentDeviceType.ts

2df172d

nsams reviewed Dec 21, 2023

View reviewed changes

packages/admin/admin/src/hooks/useCurrentDeviceType.ts Outdated Show resolved Hide resolved

jomunker added 2 commits December 21, 2023 17:45

feat(Tabs): remove dividerComponent prop

900eff6

feat(CustomTab): rename to Tab

69184c3

jomunker requested a review from johnnyomair December 22, 2023 07:39

thomasdax98 reviewed Dec 27, 2023

View reviewed changes

jomunker and others added 3 commits January 3, 2024 11:58

remove(StatusBadge): redundant todo

594e93b

Co-authored-by: Thomas Dax <thomas.dax@vivid-planet.com>

refactor(Tab): rename tabIcon to icon to avoid compatibility issues

eeb52bb

feat(RouterTabs): use top level tabComponent prop to not cause compat…

de16ff7

…ibility issues

vivid-planet deleted a comment from netlify bot Jan 10, 2024

johnnyomair marked this pull request as draft January 10, 2024 15:53

johnnyomair removed their request for review January 10, 2024 15:53

johnnyomair closed this Jan 21, 2025

johnnyomair deleted the COM-60-tab-additional-features branch January 21, 2025 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COM-60: Additional features for Tabs #1392

COM-60: Additional features for Tabs #1392

jomunker commented Nov 13, 2023 •

edited

Loading

lllHuber commented Nov 16, 2023

thomasdax98 commented Nov 16, 2023

johnnyomair commented Nov 30, 2023

johnnyomair left a comment

johnnyomair Nov 30, 2023

thomasdax98 Dec 5, 2023

jomunker Dec 22, 2023

johnnyomair Nov 30, 2023

jomunker Dec 21, 2023

thomasdax98 Dec 27, 2023

johnnyomair Nov 30, 2023

johnnyomair Nov 30, 2023

johnnyomair Nov 30, 2023

jomunker Dec 21, 2023

jomunker commented Dec 21, 2023 •

edited

Loading

jomunker commented Dec 21, 2023

thomasdax98 Dec 27, 2023

thomasdax98 Dec 27, 2023

thomasdax98 Dec 27, 2023

thomasdax98 Dec 27, 2023

thomasdax98 Dec 27, 2023

thomasdax98 Dec 27, 2023

thomasdax98 Dec 27, 2023

jomunker Jan 3, 2024

thomasdax98 Dec 27, 2023

jomunker Jan 3, 2024

jomunker Jan 3, 2024

johnnyomair commented Jan 10, 2024

johnnyomair commented Jan 21, 2025

		showStatusIcon?: boolean;
		statusIcon?: React.FunctionComponentElement<IconProps> \| undefined;

		if (!status && (showStatusIcon \|\| statusIcon)) console.warn("A status (prop: status) has to be provided, if the status icon should show.");

		if (status && statusIcon && !showStatusIcon) console.warn("The status icon will only be shown, if the showStatusIcon prop is set to true.");


		export const Divider: React.FC<DividerProps> = () => null;

		export const CustomDivider: React.FC<DividerProps> = ({ children, ...props }) => {


		export type DividerProps = Omit<MuiDividerProps, "orientation" \| "variant">;

		export const Divider: React.FC<DividerProps> = () => null;

COM-60: Additional features for Tabs #1392

COM-60: Additional features for Tabs #1392

Conversation

jomunker commented Nov 13, 2023 • edited Loading

Acceptance criteria

Screenshots

Implementation

Tabs

RouterTabs

lllHuber commented Nov 16, 2023

thomasdax98 commented Nov 16, 2023

johnnyomair commented Nov 30, 2023

johnnyomair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jomunker commented Dec 21, 2023 • edited Loading

jomunker commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyomair commented Jan 10, 2024

johnnyomair commented Jan 21, 2025

jomunker commented Nov 13, 2023 •

edited

Loading

jomunker commented Dec 21, 2023 •

edited

Loading