Updates to NJWDS button web component + new icon component #8

jasnoo · 2024-09-23T16:11:09Z

Description

NJWDS Web components created/updated

Button component

button components updated per design (Design)
Includes variant styling - primary, secondary, and link
Includes mode styling across all variants - light mode, dark mode, danger
Includes icon button - can be leading, trailing, or icon only button
Previously implemented version included here: Implement basic njwds-button component #4

Icon component

icon component (Design)
Implemented in the button component

The core of the changes that are actually made in the repo would be the following

packages/stencil-library/src/components/button/button.tsx - button component
packages/stencil-library/src/components/button/button.e2e.ts - button component tests
packages/stencil-library/src/components/icon/icon.tsx - icon component test
packages/stencil-library/src/components/icon/icon.e2e.ts and packages/stencil-library/src/components/icon/icon.spec.ts - icon component tests

Note these are generated during the build process

packages/stencil-library/src/components.d.ts
packages/react-library/ directory

Steps to Test

To run in dev:

npm i
Go into packages/stencil-library directory > npm run start

To test:

In packages/stencil-library directory > npm run test

ezhangy

Looks good to me! A couple small comments and a couple things I think we should document as issues and return to in the future.

packages/stencil-library/src/components/button/button.e2e.ts

packages/stencil-library/src/components/button/button.tsx

ezhangy · 2024-09-24T16:12:37Z

packages/stencil-library/src/components/button/button.tsx

-                    return "usa-button--unstyled usa-button--outline usa-button--inverse"
+                    return "usa-button--unstyled unstyled-button-dark"
+                default:
+                    return 'primary-button-dark'


[dust] I think it might be a good idea to add "usa" or "nj" as a prefix to these custom classes in order to avoid styling naming conflicts in projects.

Maybe we can create an issue in the repo to track for later?

I think issue will be good here, we should align this across the njwds repo as well

ezhangy · 2024-09-24T16:25:39Z

packages/stencil-library/src/components/button/button.e2e.ts

+    return element.className.split(' ').sort();
+};
+
+const variant: Record<ButtonVariant, ButtonVariant> = {


[dust] I really like the pattern of using Record objects for variant, etc. to make the tests clearer!

packages/stencil-library/src/components/icon/icon.e2e.ts

packages/stencil-library/src/components/icon/icon.tsx

ezhangy · 2024-09-24T16:43:10Z

packages/react-library/lib/components/stencil-generated/react-component-lib/attachProps.ts

[dust] just confirming all of the new files in the react-component-lib directory were auto-generated by stencil? I assumed this was the case so I didn't check any of these files.

Yep, they were generated by stencil!

packages/stencil-library/src/index.html

namanaman

Looks great, thorough testing!

namanaman · 2024-09-25T20:12:15Z

packages/stencil-library/src/interface.d.ts

@@ -1 +1,3 @@
-export type Mode = 'light' | 'dark';
+export type Mode = 'light' | 'dark' | 'danger';


[Dust] Why is danger a mode rather than a variant?

This aligns with the design where there's the 3 variants (primary/secondary/link) and each has a version of each mode.

namanaman · 2024-09-25T20:17:16Z

packages/stencil-library/src/components/button/button.tsx


    private getButtonClassName(): string {
+
        const getVariantClassName = (variant: ButtonVariant): string => {


[Dust] Should be this be named getLightModeVariantClassName to align with the other 2?

namanaman · 2024-09-25T20:18:12Z

packages/stencil-library/src/components/button/button.tsx

-        return this.mode === "light"
-            ? `usa-button ${getVariantClassName(this.variant)}`
-            : `usa-button ${getDarkModeVariantClassName(this.variant)}`
+        let getClassName;


[Dust] IMO setting the value of the classname (by invoking the function) rather than setting the function to call may be clearer for a dev

namanaman · 2024-09-25T20:20:01Z

packages/stencil-library/src/components/button/button.tsx

+        let iconClass = ''
+        switch (this.iconPosition) {
+            case "leading":
+                iconClass += ' margin-right-105'


[Dust] Why += vs. just setting it directly? Since the initial class is empty

Good call, originally I had other text in iconClass but I took it out

namanaman · 2024-09-25T20:21:39Z

packages/stencil-library/src/components/button/button.tsx

+            if (this.iconPosition === 'icon-only') {
+                return (
+                  <njwds-icon icon={this.icon} size="3" decorative={false} iconTitle={this.iconTitle}></njwds-icon>
+                )

+            } else {
+                return (
+                  <njwds-icon class={iconClass} icon={this.icon} size="3" decorative={true} iconTitle={this.iconTitle}></njwds-icon>
+                )
+            }


[Dust] I'm wondering if it'd be clearer if we just had a single return statement and we conditionally change the attribute for decorative and class so we don't need to repeat the rest of the attributes for both. It may be clearer to the dev which things are static and which are changing

namanaman · 2024-09-25T20:22:23Z

packages/stencil-library/src/components/button/button.e2e.ts

[Praise] Such a comprehensive test, nice!

namanaman · 2024-09-25T20:26:27Z

packages/stencil-library/src/components/icon/icon.tsx

+    @Prop() decorative: boolean = false;
+    @Prop() iconTitle?: string
+
+    componentWillLoad() {


[Dust] Any reason why this falls in componentWillLoad vs. another lifecycle method?

namanaman · 2024-09-25T20:27:17Z

packages/stencil-library/src/components/icon/icon.tsx

+        const sizeToClass = {
+            "scale": "",
+            "3": "usa-icon--size-3",
+            "4": "usa-icon--size-4"
+        }


[Pebble] This could be a constant outside of the component scope to make it clearer that it's not dependent on the internals of the function or class

namanaman · 2024-09-25T20:29:18Z

packages/stencil-library/src/components/icon/icon.tsx

+        return this.decorative
+            ? (
+                <svg class={iconClass} aria-hidden="true" focusable="false" role="img">
+                    <use xlinkHref={iconSrc}></use>
+                </svg>
+            )
+            : (
+                <svg class={iconClass} aria-labelledby={iconTitleId} role="img" focusable="false">
+                    <title id={iconTitleId}>{this.iconTitle ?? this.icon}</title>
+                    <use xlinkHref={iconSrc}></use>
+                </svg>
+            )


[Dust] Similar question as button icon where can we return just 1 thing and conditionally adjust the attributes that are dynamic within there? This would make it easier to see what's constant and also mean if we change something in a constant part (use xlinkHref) in one place, we don't have to do it twice.

ezhangy and others added 30 commits August 1, 2024 14:04

wip: add button tests for variant prop

357e48d

refactor: rename boolean button props to look more like HTML attributes

892ebd7

refactor: button component to only have variant prop

28b0469

tests: add button tests to check as-child attribute

9747ef6

chore: delete unnecessary buttonAttributes component

0bfcb59

chore: remove buttonAttributes from Njwdsbutton interface

e3c88b4

refactor: button component uses NJWDS naming for variant property

2ad18e7

delete autogen docs

adc123a

add components to www output

24bc49d

delete autogen readmes

0b9f0d9

add button doc page

d359281

fix: inverse needs outline class to work

8eae388

rename variants to secondary-dark and link-dark

2ee7c75

refactor variant prop to type

502811e

wip: button design review

231a208

wip: update button HTML docs

5ecd4d8

add mode prop

269ec2f

Merge branch 'main' into button-component-tests

09cf194

basic icon component (small size unsupported)

2110fb6

add icon tests

4097915

refactor: remove asChild prop on button component

a29ad2c

Add additional variants and styling to button

478f4f4

Icon component update

d238b0d

Updated stencil components

30c3090

updated icon only label and button tests

71c9e31

Icon button uses icon component

a758561

Updating generated react-library files

caa8d0b

Fixed to ts file

b0c56fa

Merge branch 'main' into button-component-tests

619750d

Removed default template component

0b887a7

jasnoo changed the title ~~Updates to NJWDS button web component + icon component~~ Updates to NJWDS button web component + new icon component Sep 23, 2024

jasnoo requested review from ezhangy and namanaman September 23, 2024 17:24

ezhangy approved these changes Sep 24, 2024

View reviewed changes

jasnoo added 4 commits September 24, 2024 13:38

removed file

a54b35f

test fixes

22c4310

removed additional sizes for icon

1082e14

Updated HTML documentation for button + icon

b7221c7

namanaman approved these changes Sep 25, 2024

View reviewed changes

jasnoo added 3 commits September 26, 2024 08:59

Updated icon classes

44bddab

Updated lightmode funciton name

2686d27

Fixed spacing

1aabce6

jasnoo merged commit ebd53b4 into main Sep 26, 2024
1 check passed

jasnoo deleted the button-component-tests branch September 26, 2024 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to NJWDS button web component + new icon component #8

Updates to NJWDS button web component + new icon component #8

jasnoo commented Sep 23, 2024 •

edited

Loading

ezhangy left a comment

ezhangy Sep 24, 2024

jasnoo Sep 24, 2024

ezhangy Sep 24, 2024

ezhangy Sep 24, 2024

jasnoo Sep 24, 2024

namanaman left a comment

namanaman Sep 25, 2024

jasnoo Sep 26, 2024

namanaman Sep 25, 2024

namanaman Sep 25, 2024

namanaman Sep 25, 2024

jasnoo Sep 26, 2024

namanaman Sep 25, 2024

namanaman Sep 25, 2024

namanaman Sep 25, 2024

namanaman Sep 25, 2024

namanaman Sep 25, 2024

		@@ -1 +1,3 @@
		export type Mode = 'light' \| 'dark';
		export type Mode = 'light' \| 'dark' \| 'danger';


		private getButtonClassName(): string {

		const getVariantClassName = (variant: ButtonVariant): string => {

Updates to NJWDS button web component + new icon component #8

Updates to NJWDS button web component + new icon component #8

Conversation

jasnoo commented Sep 23, 2024 • edited Loading

Description

Steps to Test

ezhangy 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

namanaman 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

Choose a reason for hiding this comment

jasnoo commented Sep 23, 2024 •

edited

Loading