feat(components): added a component called Tabs #15437

syao1226 · 2024-06-17T18:54:09Z

Overview

RSQ-86: Create a new Tabs component to unify the TabbedButton/RoundTab components in storybook.

https://www.figma.com/design/4gRzu3rkNDZnDPkP36YxfF/Chip%2C-Tag-and-Tab?node-id=123-57775&t=zucfumEcRony14Xm-0

Test Plan

Changelog

Review requests

Risk assessment

…tton/RoundTab components close RSQ-86

shlokamin · 2024-06-17T19:06:40Z

components/src/molecules/Tabs.tsx

+                            handleClick(index)
+                            button.onClick()  
+                        }}
+                        className={index === activeTab ? 'active' : ''}


what is this className doing? since we use styled-components instead of raw css we don't need to apply css classes do we?

shlokamin · 2024-06-17T19:09:44Z

components/src/molecules/Tabs.tsx

+                            handleClick(index)
+                            button.onClick()  


i think having the button both update its internal state (activeTab ) as well as call the provided onClick handler is a little confusing because you have two sources of truth for whether or not a button is active (this component's internal react state, as well as the provided isActive prop). i prefer having the parent component tell us whether or not a button is active, so i think we should get rid of the internal activeTab react state variable

shlokamin · 2024-06-17T19:09:55Z

components/src/molecules/Tabs.tsx

+    const [activeTab, setActiveTab] = React.useState<number | null>(null);
+
+    const handleClick = (index: number) : void => {
+        setActiveTab(index)
+    }


see comment below, i dont think we need this!

shlokamin · 2024-06-17T19:12:40Z

components/src/molecules/Tabs.tsx

+
+                        css={index === activeTab || button.isActive === true ? currentTabStyle : button.disabled === true ? disabledTabStyle : defaultTabStyle}


instead of conditionally choosing a CSS block for disabled, you can tell an HTML element that it is disabled by providing the disabled attribute: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/disabled

this will simplify the ternary 😄

…nClick state close RQS-86

jerader · 2024-06-18T17:42:39Z

components/src/molecules/Tabs.stories.tsx

+import * as React from 'react'
+import { Tabs as TabComponent } from './Tabs'
+import type { Meta, StoryObj } from '@storybook/react'
+import { useArgs } from '@storybook/preview-api';


nit: we are very particular about import order, the pattern goes:

package imports

top level directory imports (like opentrons/shared-data or opentrons/components)

imports from the farthest path to the shortest so ../../ should be above ./ for example

types (type imports should also follow the above pattern)

Suggested change

import * as React from 'react'

import { Tabs as TabComponent } from './Tabs'

import type { Meta, StoryObj } from '@storybook/react'

import { useArgs } from '@storybook/preview-api';

import * as React from 'react'

import { useArgs } from '@storybook/preview-api';

import { Tabs as TabComponent } from './Tabs'

import type { Meta, StoryObj } from '@storybook/react'

thanks jethary, i forgot to explain this to shiyao

jerader · 2024-06-18T17:43:08Z

components/src/molecules/Tabs.tsx

+import { TYPOGRAPHY, SPACING } from '../ui-style-constants'
+import { COLORS, BORDERS } from '../helix-design-system'
+import { POSITION_RELATIVE, DIRECTION_COLUMN, DIRECTION_ROW } from '../styles'
+import { Flex } from '../primitives'


import order is good here! 👍

jerader · 2024-06-18T17:43:38Z

components/src/molecules/Tabs.tsx

+        text: string
+        isActive?: boolean
+        disabled?: boolean
+        onClick: () => void


nit: optional props should go last

Suggested change

text: string

isActive?: boolean

disabled?: boolean

onClick: () => void

text: string

onClick: () => void

isActive?: boolean

disabled?: boolean

jerader · 2024-06-18T17:44:32Z

components/src/molecules/Tabs.tsx

+                        onClick={() => {
+                            button.onClick()  
+                        }}
+                        css={button.isActive === true ? currentTabStyle : defaultTabStyle}


nit: i think this works as a shorthand

Suggested change

css={button.isActive === true ? currentTabStyle : defaultTabStyle}

css={button.isActive ? currentTabStyle : defaultTabStyle}

I've tried deleting the "=== true" statement and it gives me this problem: Unexpected nullable boolean value in conditional. Please handle the nullish case explicitly.eslint@typescript-eslint/strict-boolean-expressions

i see, i guess its because isActive can be undefined. you can keep it as is!

koji · 2024-06-18T18:31:17Z

components/src/molecules/Tabs.tsx

+
+export function Tabs(props: TabsProps): JSX.Element {
+
+    const { buttons } = props


why not using interface you defined?

const = {text, onClick, isActive = false, disabled = false } = props

I have the text, onClick, isActive, and disabled wrapped inside an array in the interface because we need to show multiple buttons, so i am not able to call those props directly i think

I recommend you use tabs instead of buttons since you button tag.

I checked a couple of ui components.
they use the following structure and I think RoundTab is also the same.
I guess passing a custom array to ui component is unusual.

<Tabs> <Tab value="1">tab1<Tab> <Tab value="2">tab2<Tab> </Tabs>

Is there any specific reason that you would want to pass an array to this component?

that's a really good question! based on what Shlok have told me we want the TabsProps take in a list of tabs, where each tab has text, isActive, isDisabled, and onClick. that way we keep the interface terse and generic

↑ any thoughts on the above?
@shlokamin @jerader @ncdiehl11

https://github.com/reactjs/react-tabs
https://mui.com/material-ui/react-tabs/#basics
https://v2.chakra-ui.com/docs/components/tabs/usage#fitted-tabs

i think something we can do to make it cleaner is create a new type that includes

text: string isActive?: boolean disabled?: boolean onClick: () => void

So for example:

interface TabProps { text: string isActive?: boolean disabled?: boolean onClick: () => void }

And then

export interface TabsProps { tabs: TabProps[] }

Because this is the Tabs component that has Tab[] inside of it, i think instead of calling it buttons, it could be tabs?

koji · 2024-06-18T18:53:03Z

components/src/molecules/Tabs.tsx

+import { POSITION_RELATIVE, DIRECTION_COLUMN, DIRECTION_ROW } from '../styles'
+import { Flex } from '../primitives'
+
+const defaultTabStyle = css`


basically we use SCREAMING_SNAKE_CASE for a css var.

Suggested change

const defaultTabStyle = css`

const DEFAULT_TAB_STYLE = css`

lol i had no idea it was called SCREAMING_SNAKE_CASE

colse RSQ-86

close RSQ-86

koji · 2024-06-18T20:33:59Z

components/src/molecules/Tabs.tsx

unify the TabbedButton/RoundTab

This means that this new component needs to support ODD style like what Jethary does for Tag.

https://github.com/Opentrons/opentrons/pull/15441/files#diff-69b769c5ad20c1d688db697b1ea496fecad8e6dc0f020bba62e048b1c4d541d0

We use media-query to switch styles. The following for ODD
@media ${RESPONSIVENESS.touchscreenMediaQuerySpecs}

Also, could you write test for the component?

https://opentrons.atlassian.net/wiki/spaces/PAR/pages/4086104119/UI+component+Unification+Implementation+Guide

@koji when you have some time would you mind pairing with Shiyao to show her how to write a test using react testing library?

shlokamin

looks good so far shiyao! it looks like what's remaining is:

add a test for the component (@koji or @ncdiehl11 can show you how)
add ODD specific style overrides for the style differences in ODD
replace all instances of the old chip and tag with your new component (we can do this after Mel gives us the thumbs up that the component is designed to spec)

koji · 2024-06-19T01:17:46Z

@syao1226

Create Tab component and bring TabbedButton(for ODD) and RoundTab(for desktop) into Tab
TabbedButton's style should be under @media ${RESPONSIVENESS.touchscreenMediaQuerySpecs}
I recommend you use Btn component instead of button tag since we use that component.

we use the following structure for a component.
For Tab, the following is the preferred structure.

molecules
└── Tab
    ├── Tab.stories.tsx
    ├── __tests__
    │   └── Tab.test.tsx
    └── index.tsx

[samples]
https://github.com/Opentrons/opentrons/pull/15441/files#diff-69b769c5ad20c1d688db697b1ea496fecad8e6dc0f020bba62e048b1c4d541d0

https://github.com/Opentrons/opentrons/blob/edge/components/src/atoms/Chip/index.tsx

Create Tabs component that has the layout for each app(Desktop app and ODD app)
Tabs.stories.tsx should have Tabs

molecules
└── Tabs
    ├── Tabs.stories.tsx
    ├── __tests__
    │   └── Tabs.test.tsx
    └── index.tsx

Check Tab and Tabs in Storybook
They should work on Desktop app and ODD app.
These components shows different style by switching the screen size like below.

Recording.2024-06-18.211405.mp4

Write a test for each component
Nick or I can show you how to write a test for a component.
Remove RoundTab stories and TabbledButton stories

If you have any question on this, feel free to reach out to the team/me.

re RSQ-86

koji · 2024-06-21T18:38:36Z

components/src/molecules/Tabs/Tabs.stories.tsx

+    tabs: [
+      {
+        text: 'Setup',
+        isActive: false,


when the app displays tabs, setup is selected so settings isActive: true would be designer-friendly.

Suggested change

isActive: false,

isActive: true,

koji · 2024-06-21T18:42:08Z

components/src/molecules/Tabs/index.tsx

+
+const DEFAULT_TAB_STYLE = css`
+  ${TYPOGRAPHY.pSemiBold}
+  color: ${COLORS.black90};


the default text color is black90 so you don't need this.

L28 in app/src/atoms/GlobalStyle/index.ts

Suggested change

color: ${COLORS.black90};

koji · 2024-06-21T18:45:22Z

components/src/molecules/Tabs/__tests__/Tabs.test.tsx

+import { SPACING } from '../../../ui-style-constants'
+import { POSITION_RELATIVE } from '../../../styles'
+import { renderWithProviders } from '../../../testing/utils'
+import { Tabs } from '..'


Suggested change

import { Tabs } from '..'

import { Tabs } from '../index'

koji · 2024-06-21T18:54:37Z

components/src/molecules/Tabs/index.tsx

+      flexDirection={DIRECTION_COLUMN}
+      gridGap={SPACING.spacing16}
+      padding={SPACING.spacing16}


flexDirection and gridGap aren't needed since there is one element.

Could you add the Figma link you used to implement this to the pr's overview?

oh I was using the css layout in the RoundTab component

https://www.figma.com/design/4gRzu3rkNDZnDPkP36YxfF/Chip%2C-Tag-and-Tab?node-id=123-57775&t=BqW7cEUlr7vSU9f3-0

I think the style you took is for storybook's layout.
https://s3-us-west-2.amazonaws.com/opentrons-components/edge/index.html?path=/story/library-molecules-roundtab--round-tab

koji · 2024-06-21T19:02:40Z

components/src/molecules/Tabs/index.tsx

+  ${TYPOGRAPHY.pSemiBold}
+  color: ${COLORS.black90};
+  background-color: ${COLORS.purple30};
+  border: 0px ${BORDERS.styleSolid} ${COLORS.purple30};


need this line?
border uses 0px which isn't visible.

re RSQ-86

koji · 2024-06-21T20:28:20Z

@shlokamin

#15437 (comment)
Could you explain why we need to change the existing implementation to Shiyao's implementation?

shlokamin · 2024-06-21T22:50:17Z

#15437 (comment) Could you explain why we need to change the existing implementation to Shiyao's implementation?

@koji im not sure what you mean by "existing implementation". the reason i suggested that this component take in a list of tabs is because of the description of the component in Figma:

"Tabbed interfaces are a way of navigating between multiple panels".

keyword "multiple"!

we will never expose a single Tab interface, it is meant to always be a sequence of tabs (hence the component name)

shlokamin · 2024-06-21T22:51:23Z

ultimately i don't really think it matters too much though. having a single tab interface is fine too, but i personally don't see anything wrong with the interface this component currently accepts

…esktop app re RSQ-86

re RSQ-86

…o components-unify-tab-button

re RSQ-86

koji · 2024-06-27T19:02:45Z

Reference in ne

ultimately i don't really think it matters too much though. having a single tab interface is fine too, but i personally don't see anything wrong with the interface this component currently accepts

My concern is that if there is a request to change the background color of a particular tab or to display an icon, multiples would require additional changes.
I guess that would be one reason that major UI components use a single tab component and tags component to manage tab components.

shlokamin · 2024-06-27T19:10:28Z

My concern is that if there is a request to change the background color of a particular tab or to display an icon, multiples would require additional changes.

oh if there is ever a request to do that we would say we can't. that would be a big signal that this abstraction is not correct and that this component should not exist. cc @mmencarelli

re RSQ-86

koji · 2024-06-28T18:37:07Z

components/src/molecules/Tabs/Tabs.stories.tsx

+  parameters: VIEWPORT.touchScreenViewport,
+  argTypes: {
+    tabs: {
+      control: {


@syao1226 Have you researched a way to control an element in array?

This isn't must-have but nice-to-have since designers would like to use a select button for disabled instead of changing the boolean directly.

I haven't and I'll look into it now

re RSQ-86

shlokamin · 2024-07-02T20:18:33Z

components/src/molecules/Tabs/__tests__/Tabs.test.tsx

+    expect(tab2).not.toHaveStyle(`background-color: ${COLORS.purple50}`)
+    expect(tab2).not.toHaveStyle(`color: ${COLORS.white}`)
+
+    expect(tab3).not.toHaveStyle(`background-color: ${COLORS.purple50}`)
+    expect(tab3).not.toHaveStyle(`color: ${COLORS.white}`)


no need to check these since you're already asserting these same properties on lines 57-58

shlokamin

code lgtm

close RSQ-86

# Overview  RSQ-86: Create a new Tabs component to unify the TabbedButton/RoundTab components in storybook. https://www.figma.com/design/4gRzu3rkNDZnDPkP36YxfF/Chip%2C-Tag-and-Tab?node-id=123-57775&t=zucfumEcRony14Xm-0 # Test Plan  # Changelog  # Review requests  # Risk assessment  --------- Co-authored-by: shiyaochen <shiyaochen@admins-MacBook-Pro.local> Co-authored-by: shiyaochen <shiyaochen@admins-mbp.mynetworksettings.com>

feat(components): make the new Tabs component that unify the TabbedBu…

ad312a3

…tton/RoundTab components close RSQ-86

syao1226 requested review from koji, shlokamin, ncdiehl11 and jerader June 17, 2024 18:54

fixed lint.js test by adding a return value

f83675d

shlokamin reviewed Jun 17, 2024

View reviewed changes

fix(component): updating a function to handle button's isActive and o…

a6e12e9

…nClick state close RQS-86

syao1226 force-pushed the components-unify-tab-button branch from 3aeb35b to a6e12e9 Compare June 18, 2024 17:38

jerader reviewed Jun 18, 2024

View reviewed changes

koji reviewed Jun 18, 2024

View reviewed changes

shiyaochen added 2 commits June 18, 2024 14:56

fix(components): formating files

819c847

colse RSQ-86

fix(components): update naming of variables

3670c58

close RSQ-86

koji reviewed Jun 18, 2024

View reviewed changes

shlokamin reviewed Jun 18, 2024

View reviewed changes

koji added the authorship label Jun 19, 2024

shiyaochen and others added 2 commits June 20, 2024 09:46

feat(components): adding TabbedButton's style for ODD

10d13e2

re RSQ-86

test(components): write a test for Tabs component

e30836a

re RSQ-86

syao1226 marked this pull request as ready for review June 21, 2024 18:24

syao1226 requested a review from a team as a code owner June 21, 2024 18:24

syao1226 requested review from jerader, koji and shlokamin June 21, 2024 18:33

koji reviewed Jun 21, 2024

View reviewed changes

shiyaochen added 2 commits June 21, 2024 15:23

refactor(components): remove useless css styles

d4e083d

re RSQ-86

style(components): correct the format

bb0ab40

re RSQ-86

shiyaochen and others added 6 commits June 25, 2024 09:49

feat(components): replacing RoundTab component to Tabs component in d…

b4b8bf0

…esktop app re RSQ-86

Merge branch 'edge' into components-unify-tab-button

f8b9375

feat(components): replacing TabbedButton with Tabs

7024e4f

re RSQ-86

Merge remote-tracking branch 'origin/components-unify-tab-button' int…

bc8b3e9

…o components-unify-tab-button

style(components): format the files

14c7070

re RSQ-86

feat(components): replacing TabbedButton in ProtocolDetails with Tabs

d3af2a0

re RSQ-86

shiyaochen and others added 3 commits June 28, 2024 10:44

feat(components): change background color when hover over disabled tabs

b4a9e27

re RSQ-86

Merge branch 'edge' into components-unify-tab-button

807f446

fix(components): fix the import component

d8f0a07

re RSQ-86

koji reviewed Jun 28, 2024

View reviewed changes

shiyaochen added 4 commits July 2, 2024 10:50

refactor(components): remove RoundTab and TabbedButton files

0aff142

re RSQ-86

Merge branch 'edge' into components-unify-tab-button

4f6d592

fix(components): fix typecheck error

8e46814

re RSQ-86

fix(components): remove duplicate tab identifier

8d4d387

re RSQ-86

shlokamin reviewed Jul 2, 2024

View reviewed changes

shlokamin approved these changes Jul 2, 2024

View reviewed changes

test(components): remove useless test

044f642

close RSQ-86

syao1226 merged commit 117afb2 into edge Jul 2, 2024
48 checks passed

syao1226 deleted the components-unify-tab-button branch July 2, 2024 21:06


		css={index === activeTab \|\| button.isActive === true ? currentTabStyle : button.disabled === true ? disabledTabStyle : defaultTabStyle}

	css={button.isActive === true ? currentTabStyle : defaultTabStyle}
	css={button.isActive ? currentTabStyle : defaultTabStyle}


		export function Tabs(props: TabsProps): JSX.Element {

		const { buttons } = props

feat(components): added a component called Tabs #15437

feat(components): added a component called Tabs #15437

Conversation

syao1226 commented Jun 17, 2024 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlokamin Jun 17, 2024 • edited Loading

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

syao1226 Jun 18, 2024 • edited Loading

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

koji Jun 18, 2024 • edited Loading

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

koji Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlokamin left a comment

Choose a reason for hiding this comment

koji commented Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

koji Jun 21, 2024 • edited Loading

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

koji Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koji commented Jun 21, 2024

shlokamin commented Jun 21, 2024

shlokamin commented Jun 21, 2024

koji commented Jun 27, 2024

shlokamin commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlokamin left a comment

Choose a reason for hiding this comment

syao1226 commented Jun 17, 2024 •

edited

Loading

shlokamin Jun 17, 2024 •

edited

Loading

syao1226 Jun 18, 2024 •

edited

Loading

koji Jun 18, 2024 •

edited

Loading

koji Jun 18, 2024 •

edited

Loading

koji commented Jun 19, 2024 •

edited

Loading

koji Jun 21, 2024 •

edited

Loading

koji Jun 21, 2024 •

edited

Loading