Storybook audit #1337

shreeyash07 · 2024-09-02T10:01:27Z

Addresses:

Changes

Storybook audit major changes

Breadcrumbs component
Secondary Tab/Navigation tab
Button
Input Container height
Modal Width
MultiSelectInput

This PR doesn't introduce:

changeset-bot · 2024-09-02T10:01:30Z

⚠️ No Changeset found

Latest commit: 1bb82e5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

samshara · 2024-10-09T05:56:07Z

app/src/components/GoBreadcrumbs/index.tsx

+interface GoBreadcrumbsProps {
+    routeData: RouteData[]
+}


Suggested change

interface GoBreadcrumbsProps {

routeData: RouteData[]

}

interface GoBreadcrumbsProps {

routeData: RouteData[];

}

samshara · 2024-10-09T05:57:07Z

app/src/components/GoBreadcrumbs/index.tsx

+            keySelector={(datum) => datum.to}
+            labelSelector={(datum) => datum.label}


Let's define function and use them instead.

samshara · 2024-10-09T06:01:10Z

packages/go-ui-storybook/src/stories/Breadcrumbs.stories.tsx

+const rendererParams = () => ({});
+


Why do we need this?

samshara · 2024-10-09T06:02:09Z

packages/go-ui-storybook/src/stories/Breadcrumbs.stories.tsx

+        labelSelector,
+        rendererParams,
+        // eslint-disable-next-line react/jsx-props-no-spreading
+        renderer: (props) => <div {...props} />,


This doesn't look right

tnagorra · 2024-10-21T05:10:58Z

app/src/components/GoBreadcrumbs/index.tsx

I am not sure if changing how we use Breadcrums is a good change.
cc: @frozenhelium @samshara

@tnagorra Can you propose a better alternative. The requirement is that we need to separately style the leaf link.

Looks like the leaf link is already styled differently in current approach as well.
We can use this new approach if we want more flexibility.

tnagorra · 2024-10-21T05:12:30Z

app/src/components/domain/ActiveOperationMap/index.tsx

Was this change required on the storybook audit?

tnagorra · 2024-10-21T05:16:56Z

packages/go-ui-storybook/src/stories/index.css

+.table-link{
+    color: var(--go-ui-color-text);
+}
+.table-link:hover {
+    color: var(--go-ui-color-primary-red);
+}
+.table-header-row {
+    background:var(--go-ui-color-primary-gray-10);
+}


These styles should not be added here

tnagorra · 2024-10-21T05:58:20Z

packages/ui/src/components/Alert/styles.module.css

@@ -1,7 +1,7 @@
 .alert {
    display: flex;
    border-radius: var(--go-ui-border-radius-md);
-    box-shadow: var(--go-ui-box-shadow-2xl);
+    box-shadow: 0px 3px 8px rgba(0, 0, 0, 0.2);


We should create design tokens for these.
cc: @frozenhelium

tnagorra · 2024-10-21T05:59:53Z

packages/ui/src/components/Alert/index.tsx

I am not sure how the following feature are implemented in this file:

Remove the header on Alert popup

Add a "x" button to close the alert

Reference: #1291

tnagorra · 2024-10-21T06:01:38Z

packages/ui/src/components/BarChart/styles.module.css

@@ -20,13 +20,13 @@
        }

        .bar-track {
-            border-radius: 0.3rem;
+            border-radius: 0 var(--go-ui-border-radius-md) var(--go-ui-border-radius-md) 0;


Is --go-ui-border-radius-md equal or close to 0.3rem?

No, it's not, but the requirement was to remove border from the left side of the bar

tnagorra · 2024-10-21T06:07:30Z

packages/ui/src/components/RadioInput/Radio/styles.module.css

        font-size: var(--go-ui-height-icon-multiplier);
    }

    .content {
        flex-direction: column;
        gap: 0;
        line-height: var(--go-ui-line-height-sm);
+        color: var(--go-ui-color-black);


We should not hard-code this change here. The suggestion most probably applies to the whole site.
cc: @frozenhelium

The change is for the whole site

tnagorra · 2024-10-21T06:14:03Z

packages/ui/src/components/Button/styles.module.css

    text-transform: var(--text-transform);
-    line-height: var(--go-ui-line-height-xs);
+    line-height: 21px;


Not sure this is the correct way. Also, let's use design tokens.
cc: @frozenhelium

tnagorra · 2024-10-21T06:17:22Z

packages/go-ui-storybook/src/stories/Button.stories.ts

+export const ProcessButtonWithIcon: Story = {
+    args: {
+        name: 'button',
+        variant: 'secondary',
+        children: 'Process Button',
+        // icons:<LoaderLineIcon/>
+    },
+};


The story for "process" buttion is not correct.

tnagorra · 2024-10-21T06:19:47Z

packages/ui/src/components/Button/index.tsx


 const buttonVariantToClassNameMap: Record<ButtonVariant, string> = {
    primary: styles.primary,
    secondary: styles.secondary,
    tertiary: styles.tertiary,
    'tertiary-on-dark': styles.tertiaryOnDark,
    'dropdown-item': styles.dropdownItem,
+    process: styles.process,


Not sure if we have this style already. But, implementing "process buttion" is adding props to indicate loading and completed state in a button.

tnagorra · 2024-10-21T06:23:47Z

packages/ui/src/components/ConfirmButton/styles.module.css

+.disabled {
+    border-color: var(--go-ui-color-gray-40);
+    background-color: var(--go-ui-color-gray-40);
+    color: var(--go-ui-color-black);
+}


This is not correct. The styling should be handled by the Button component itself.

tnagorra · 2024-10-21T06:31:21Z

packages/ui/src/components/InputContainer/styles.module.css

+            padding-left:12px;
+            height: 24px;


@frozenhelium Not sure if this is the way to set fixed height for inputs.
Also we should use design tokens.

It was requested by the auditor

tnagorra · 2024-10-21T06:31:29Z

packages/ui/src/components/InputContainer/styles.module.css

@@ -8,9 +8,10 @@
        border-radius: var(--go-ui-border-radius-md);
        background-color: var(--go-ui-color-element-background);
        padding: 0 var(--go-ui-spacing-sm);
-
+        


Suggested change

tnagorra · 2024-10-21T06:32:47Z

packages/ui/src/components/InputLabel/styles.module.css

@@ -1,6 +1,5 @@
 .input-label {
    display: flex;
-    padding: 0 var(--go-ui-spacing-2xs);


Shouldn't we be adding instead of removing the padding? Has this been defined somewhere else now?

It was removed to maintain label and value spacing as mentioned by the auditor

tnagorra · 2024-10-22T05:24:51Z

packages/ui/src/components/Header/index.tsx

@@ -80,10 +83,12 @@ function Header(props: Props) {
                            {headingDescription}
                        </div>
                    )}
+                    {modalHeading && <div className={styles.border} />}


We don't need this change. We can just use withHeaderBorder prop

tnagorra · 2024-10-22T05:25:07Z

packages/ui/src/components/Header/styles.module.css

@@ -1,4 +1,8 @@
 .header {
    display: flex;
    flex-direction: column;
+
+    .border {


We don't need this change. We can just use withHeaderBorder prop

tnagorra · 2024-10-22T05:26:56Z

packages/ui/src/components/Heading/index.tsx

@@ -25,6 +25,7 @@ export interface Props {
    level?: HeadingLevel;
    children: ReactNode;
    ellipsize?: boolean;
+    modalHeading?: boolean;


Instead of adding modalHeading, should use className to override the className

tnagorra · 2024-10-22T05:27:12Z

packages/ui/src/components/Heading/styles.module.css

+
+    &.modal-font-weight {
+        font-weight: var(--go-ui-font-weight-medium);
+    }


Instead of adding modalHeading, should use className to override the className

tnagorra · 2024-10-22T05:27:38Z

packages/ui/src/components/Container/index.tsx

@@ -30,6 +30,7 @@ export interface Props {
    className?: string;
    contentViewType?: 'grid' | 'vertical' | 'default';
    ellipsizeHeading?: boolean;
+    modalHeading?: boolean;


We don't need this change. We can just use withHeaderBorder prop

tnagorra · 2024-10-22T06:10:11Z

packages/ui/src/components/LegendItem/index.tsx

+            {icons && (
+                <div>
+                    {icons}
+                </div>
+            )}


We already have iconSrc. We don't need icons prop

iconSrc is for image path, icons prop is the for the icons, types are different

tnagorra · 2024-10-22T06:13:51Z

packages/ui/src/components/DefaultMessage/styles.module.css

+    &.empty-message {
+        padding-top: 40px;
+        height: 200px;
+    }


This styling applied for all cases.
Use design tokens.
We need a padding of 40px between the main text and description

tnagorra · 2024-10-22T06:14:28Z

packages/ui/src/components/DefaultMessage/index.tsx

                className,
            )}
-            icon={<AnalysisIcon />}
+            icon={(!empty || filtered || errored) && <AnalysisIcon />}


Not sure how this logic works!

tnagorra · 2024-10-22T06:16:17Z

packages/ui/src/components/Modal/index.tsx

@@ -93,6 +93,7 @@ function Modal(props: Props) {
                <Container
                    // eslint-disable-next-line react/jsx-props-no-spreading
                    {...containerProps}
+                    modalHeading


Looks like all headers on modal/popup should have weight 500.
We don't need modalHeading to update the weight. Also, modalHeading in context of Container does not make sense.

tnagorra · 2024-10-22T06:17:28Z

packages/ui/src/components/Modal/styles.module.css

-            width: 100%;
+            width: 420px;


Let's use design tokens. Also, the width should be set on the modal-container
cc: @frozenhelium ?

tnagorra · 2024-10-22T06:27:47Z

packages/ui/src/components/SearchMultiSelectInput/index.tsx

    return (
        <SelectInputContainer
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...otherProps}
            name={name}
            options={realOptions}
            optionsPending={optionsPending}
+            selectedOptions={selectedOptions}


Why was this line added? Was there previously a bug?

No, it was already there.

tnagorra · 2024-10-22T06:30:00Z

packages/ui/src/components/SelectInputContainer/i18n.json

        "buttonTitleClear":"Clear",
+        "buttonClearAll":"Clear All",


Why do we need 2 labels for "Clear"

tnagorra · 2024-10-22T06:30:50Z

packages/ui/src/components/SelectInputContainer/index.tsx

-            if (!persistentOptionPopup) {
-                handleHideDropdown();
-            }


Why remove handling of persistentOptionPopup? This should either be removed from the props or not removed at all.

tnagorra · 2024-10-22T06:32:13Z

packages/ui/src/components/SelectInputContainer/index.tsx

+                        {!readOnly && onSelectAllButtonClick
+                        && <div className={styles.clearAllBorder} />}


The formatting is not clear

tnagorra · 2024-10-22T06:32:33Z

packages/ui/src/components/SelectInputContainer/index.tsx

@@ -355,9 +335,60 @@ function SelectInputContainer<
                    parentRef={inputSectionRef}
                    className={_cs(optionsPopupClassName, styles.popup)}
                >
+                    <div className={styles.clearButtonContainer}>


This is not just clearButtionContainer

tnagorra · 2024-10-22T06:34:53Z

packages/ui/src/components/SelectInputContainer/index.tsx

+    const availableOptions = options.filter(
+        (option) => !selectedOptions?.some(
+            (selectedOption) => optionKeySelector(
+                selectedOption,
+                0,
+            ) === optionKeySelector(option, 0),
+        ),
+    );


Let's use a dict to filter these options. Also, add note what available options are.
Also, is this change correct for single select inputs?

tnagorra · 2024-10-22T06:35:58Z

packages/ui/src/components/SelectInputContainer/styles.module.css

+        .clear-all-border {
+            border: var(--go-ui-width-separator-thin) solid var(--go-ui-color-separator);
+        }


This is not a border. It's a separator.

tnagorra · 2024-10-22T06:41:21Z

packages/ui/src/components/SearchMultiSelectInput/index.tsx

+    const handleSelectAll = useCallback(
+        () => {
+            if (isNotDefined(options)) {
+                return;
+            }

+            const allValues = options.map(keySelector);
+            onChange(allValues, name);
+        },
+        [options, name, onChange, keySelector],
+    );


Select All cannot be added to search multi select inputs.

tnagorra · 2024-10-22T06:46:08Z

packages/ui/src/components/PieChart/styles.module.css


        .legend-item {
+            text-transform: uppercase;


It's not mentioned as a changed but the example image has uppercase words.

No, Change for text style has been mentioned

tnagorra · 2024-10-22T06:47:32Z

packages/ui/src/components/NavigationTabList/styles.module.css

+            border-bottom: var(--go-ui-width-separator-thin) solid var(--go-ui-color-separator);
+            padding: 0 0 2px 0;


Why was this changed? Also use design tokens

It was changed for the new secondary variant design.

tnagorra · 2024-10-22T08:06:39Z

packages/ui/src/components/SegmentInput/styles.module.css

@@ -3,7 +3,7 @@

    .segment-list {
        flex-wrap: nowrap;
-        border: var(--go-ui-width-separator-thin) solid var(--go-ui-color-separator);
+        border:#F7F7F7 solid var(--go-ui-color-separator);


Use design tokens. Also, we don't need a border at all.

tnagorra · 2024-10-22T08:07:02Z

packages/ui/src/components/SegmentInput/styles.module.css

@@ -13,7 +13,7 @@

    .segment {
        border-radius: var(--go-ui-border-radius-full);
-        padding: var(--go-ui-spacing-3xs) var(--go-ui-spacing-md);
+        padding: var(--go-ui-spacing-3xs) var(--go-ui-spacing-md); 


Suggested change

padding: var(--go-ui-spacing-3xs) var(--go-ui-spacing-md);

padding: var(--go-ui-spacing-3xs) var(--go-ui-spacing-md);

tnagorra · 2024-10-22T08:16:44Z

packages/ui/src/components/Table/TableBodyContent/styles.module.css

-        padding: var(--go-ui-spacing-sm);
+        padding: 12px;


Let's use design tokens.

Also not sure if "top and bottom lines Indents must be at least 12 px." means adding padding

tnagorra · 2024-10-22T08:18:42Z

packages/go-ui-storybook/src/stories/Table.stories.tsx

@@ -302,5 +310,6 @@ export const WithResizableColumn: Story = {
        keySelector,
        caption: 'You can utilize the header column to adjust the width of each column.',
        resizableColumn: true,
+        headerRowClassName: 'table-header-row',


This is not correct. The header row color should be updated in the Table component itself.

tnagorra · 2024-10-22T08:20:33Z

app/src/components/NavigationTab/index.tsx

@@ -188,6 +188,7 @@ function NavigationTab(props: Props) {
            )}
            <div className={styles.childrenWrapper}>
                {children}
+                <div className={styles.activeChildrenBorder} />


Not sure if this is the right approach.

This should only be added on certain variants.

It's only for primary variant. Styles condition are in corresponding styles file

tnagorra · 2024-10-22T08:28:27Z

packages/ui/src/components/Tabs/Tab/styles.module.css

+                position: absolute;
+                left: 45%;


Not sure if this works @frozenhelium

Looks like it works!

tnagorra · 2024-10-22T08:28:58Z

packages/ui/src/components/Tabs/TabList/styles.module.css

    &.secondary {
        display: flex;
        overflow-x: auto;

-        .content {


Is this not used?

yes, this is not used

frozenhelium · 2024-10-23T08:41:34Z

packages/ui/src/components/InfoPopup/index.tsx

 import Container from '#components/Container';
 import DropdownMenu from '#components/DropdownMenu';

 import styles from './styles.module.css';

-export interface Props {
+export interface Props<N> {
+    name: N;


We don't need name props for the InfoPopup

shreeyash07 temporarily deployed to test September 2, 2024 10:01 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 2, 2024 10:02 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 2, 2024 10:03 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 3, 2024 08:12 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 3, 2024 08:13 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 3, 2024 08:14 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 9, 2024 10:44 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 9, 2024 10:45 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 9, 2024 10:46 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 10, 2024 06:10 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 10, 2024 06:11 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 10, 2024 06:12 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 10, 2024 06:45 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 10, 2024 06:46 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 10, 2024 06:47 — with GitHub Actions Inactive

shreeyash07 force-pushed the fix/storybook-ui-audit branch from 081970f to f5ba758 Compare September 13, 2024 09:15

shreeyash07 temporarily deployed to test September 13, 2024 09:15 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 13, 2024 09:16 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test September 13, 2024 09:17 — with GitHub Actions Inactive

shreeyash07 had a problem deploying to test September 16, 2024 10:24 — with GitHub Actions Failure

roshni73 had a problem deploying to test September 16, 2024 11:41 — with GitHub Actions Failure

roshni73 had a problem deploying to test September 17, 2024 03:52 — with GitHub Actions Failure

roshni73 had a problem deploying to test September 17, 2024 08:15 — with GitHub Actions Failure

roshni73 had a problem deploying to test September 18, 2024 11:17 — with GitHub Actions Failure

shreeyash07 had a problem deploying to test September 19, 2024 11:13 — with GitHub Actions Failure

shreeyash07 had a problem deploying to test September 23, 2024 05:17 — with GitHub Actions Failure

shreeyash07 had a problem deploying to test September 23, 2024 06:05 — with GitHub Actions Failure

roshni73 had a problem deploying to test September 23, 2024 06:27 — with GitHub Actions Failure

roshni73 force-pushed the fix/storybook-ui-audit branch from b0c1c57 to 314b261 Compare September 23, 2024 06:30

roshni73 temporarily deployed to test October 2, 2024 11:13 — with GitHub Actions Inactive

Update selectall button for multiselectinput

9a559ef

shreeyash07 force-pushed the fix/storybook-ui-audit branch from f3940d2 to 9a559ef Compare October 8, 2024 04:22

shreeyash07 temporarily deployed to test October 8, 2024 04:22 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test October 8, 2024 04:23 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test October 8, 2024 04:24 — with GitHub Actions Inactive

Update orientaition of the dropdown

61b5625

shreeyash07 temporarily deployed to test October 8, 2024 05:37 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test October 8, 2024 05:38 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test October 8, 2024 05:39 — with GitHub Actions Inactive

samshara requested changes Oct 9, 2024

View reviewed changes

Add Key and label selector in breadcrumbs

36494fc

shreeyash07 temporarily deployed to test October 17, 2024 10:22 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test October 17, 2024 10:23 — with GitHub Actions Inactive

shreeyash07 temporarily deployed to test October 17, 2024 10:25 — with GitHub Actions Inactive

Fix renderParams in stories

22fd1d4

roshni73 requested a review from samshara October 18, 2024 05:51

tnagorra requested changes Oct 21, 2024

View reviewed changes

tnagorra requested changes Oct 22, 2024

View reviewed changes

tnagorra reviewed Oct 22, 2024

View reviewed changes

tnagorra requested changes Oct 22, 2024

View reviewed changes

frozenhelium reviewed Oct 23, 2024

View reviewed changes

shreeyash07 and others added 2 commits October 23, 2024 16:51

Update and add missing design tokens

a892214

Fix table stories and default message

1bb82e5

roshni73 requested review from tnagorra and frozenhelium November 15, 2024 04:11

		keySelector={(datum) => datum.to}
		labelSelector={(datum) => datum.label}

		{!readOnly && onSelectAllButtonClick
		&& <div className={styles.clearAllBorder} />}

		border-bottom: var(--go-ui-width-separator-thin) solid var(--go-ui-color-separator);
		padding: 0 0 2px 0;

	padding: var(--go-ui-spacing-3xs) var(--go-ui-spacing-md);
	padding: var(--go-ui-spacing-3xs) var(--go-ui-spacing-md);

Storybook audit #1337

Are you sure you want to change the base?

Storybook audit #1337

Conversation

shreeyash07 commented Sep 2, 2024 • edited by udaynwa Loading

Addresses:

Changes

This PR doesn't introduce:

changeset-bot bot commented Sep 2, 2024 • edited Loading

⚠️ No Changeset found

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

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

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

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

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

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

shreeyash07 commented Sep 2, 2024 •

edited by udaynwa

Loading

changeset-bot bot commented Sep 2, 2024 •

edited

Loading