project: Storybook audit by shreeyash07 · Pull Request #1337 · IFRCGo/go-web-app

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: a3ecadf

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

    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


        .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

        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-align: center;
    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

    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

        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

                            {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

    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

    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

    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

                <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

            box-shadow: var(--go-ui-box-shadow-2xl);
            background-color: var(--go-ui-color-white);
-            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

            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

                    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

+        gap: var(--go-ui-border-radius-lg);

        .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

    .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

    .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

 .row {
    .cell {
-        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

        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

            )}
            <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

        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


-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 and others added 15 commits September 23, 2024 16:28

Add Missing stories and audit storybook

a57a856

Update secondary tab new design

63a82a4

change the styling of ui component stories

8ac1ebd

Add Close Button for infopopup component

333dbf7

Add gray tertiary button for button component

b5288ad

Update breadcrumb component

e5f5757

Update MultiSelectInput and InputContainer

f9f2d03

fix ActiveOperation filter action

102a4d7

fix header font size and table label color

27a5b9b

Update Secondary tab stories for tab component

3a1ca4b

Add GoBreadcrumbs component

fd5e894

Update footer stories and checkboxiconfill

98cfa26

Add select all and clear all button for Multiselect Input

72ab31f

Update selectall button for multiselectinput

9a559ef

Update orientaition of the dropdown

61b5625

samshara suggested changes Oct 9, 2024

View reviewed changes

shreeyash07 and others added 2 commits October 17, 2024 16:03

Add Key and label selector in breadcrumbs

36494fc

Fix renderParams in stories

22fd1d4

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 3 commits October 23, 2024 16:51

Update and add missing design tokens

a892214

Fix table stories and default message

1bb82e5

Fix textinput, validate button, dateinput

a3ecadf

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

		background-color: var(--go-ui-color-element-background);
		padding: 0 var(--go-ui-spacing-sm);

		{!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);

Conversation

shreeyash07 commented Sep 2, 2024 • edited by udaynwa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Addresses:

Changes

This PR doesn't introduce:

Uh oh!

changeset-bot bot commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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