Button: enhancements with ButtonLink and IconButton #1659

pksjce · 2021-11-26T12:53:27Z

Describe your changes here.

Add two new components

IconButton
ButtonLink

Change implementation of button to better accomodate sharing of code.

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

package.json

src/NewButton/types.ts

changeset-bot · 2021-11-26T13:16:45Z

⚠️ No Changeset found

Latest commit: ceb847c

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

github-actions · 2021-11-26T13:26:11Z

size-limit report 📦

Path	Size
dist/browser.esm.js	57.26 KB (+0.02% 🔺)
dist/browser.umd.js	57.59 KB (-0.01% 🔽)

rezrah · 2021-11-29T10:28:41Z

src/NewButton/icon-button.tsx

@@ -0,0 +1,31 @@
+import React, {forwardRef} from 'react'


Should this - and the other files that export default React components - be renamed in PascalCase? Or is this intentional?

Ah I see! Will change up the file names in a followup PR 👍

rezrah · 2021-11-29T10:29:34Z

src/NewButton/icon-button.tsx

+  ])
+  return (
+    <Base sx={sxStyles} ref={forwardedRef} {...props}>
+      <span hidden={true}>{iconLabel}</span>


Is hidden needed anymore? If so, should it be aria-hidden instead?

Since its an icon only component, the idea was to have a visibly hidden icon label but the screenreaders will be able to access it.

Okay, I believe the hidden html5 attribute will also hide it from screenreaders too though, because it prevents the semantics from getting surfaced to the accessibility api . I think you'll either need to just visually hide it with styles, or explicitly add aria-hidden="false", but the latter seems counter-productive.

rezrah · 2021-12-10T10:32:15Z

src/NewButton/styles.ts

+    default: {
+      color: 'btn.text',
+      backgroundColor: 'btn.bg',
+      boxShadow: `${theme?.shadows.btn.shadow}, ${theme?.shadows.btn.insetShadow}`,


is theme assumed to always be defined inside the fn? If so, do we need the optional chaining? If not, should we provide a fallback?

Honestly I only used it to make typescript happy. This seems common usage across our codebase.

rezrah · 2021-12-10T10:38:42Z

src/NewButton/styles.ts

+        color: 'btn.primary.disabledText',
+        backgroundColor: 'btn.primary.disabledBg'
+      },
+      '[data-component="ButtonCounter"]': {


Minor observation: I can't see it in the test file, but given we're applying design logic against DOM node attributes, shall we add some extra tests against the data-* attributes to ensure that these styles are picked up?

yes, some counter tests are much needed.

rezrah · 2021-12-10T11:06:28Z

src/NewButton/styles.ts

+
+      '&:hover': {
+        color: 'btn.outline.hoverText',
+        backgroundColor: 'btn.outline.hoverBg',


Potentially unrelated to this PR, but should this hover style be disabled?

Screen.Recording.2021-12-10.at.10.59.41.mov

Thanks a ton for this catch! Fixed now.

rezrah · 2021-12-10T11:18:19Z

src/__tests__/NewButton.test.tsx

  })
  it('styles icon only button to make it a square', () => {
-    const IconOnlyButton = render(<Button icon={SearchIcon}>Search icon only button</Button>)
+    const IconOnlyButton = render(<IconButton icon={SearchIcon} iconLabel="Search icon only button" />)


One for another time, but wanted to put it on your radar. Can we move away from Enzyme to use the render from testing library instead? It's already imported here as HTMLRender. On the disabled test for example, we can instead look for disabled html attributes instead of testing the implementation props. What do you think?

Good idea! Its much nicer to use testing library. Rewrote the tests in that.

rezrah

Looks great @pksjce. I've left additional questions/recommendations but would be happy for them to be resolved later if you want to move ahead with this.

1. Bug with disabled 2. Tests in testing lib

* Create icon only button * Add ButtonLink and IconButton components * Lint and test * Remove react router for now. Maybe add an example later * Fix the typescript error on as prop * Fix lint issue * Fix 1. Bug with disabled 2. Tests in testing lib * lint issues

pksjce requested review from a team and rezrah November 26, 2021 12:53

pksjce commented Nov 26, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

pksjce commented Nov 26, 2021

View reviewed changes

src/NewButton/types.ts Outdated Show resolved Hide resolved

pksjce commented Nov 26, 2021

View reviewed changes

src/NewButton/types.ts Show resolved Hide resolved

github-actions bot deployed to Preview November 26, 2021 13:16 View deployment

pksjce added react skip changeset This change does not need a changelog labels Nov 26, 2021

github-actions bot deployed to Preview November 26, 2021 13:23 View deployment

rezrah reviewed Nov 29, 2021

View reviewed changes

pksjce force-pushed the pk/button-enhancements branch from 19aca23 to 14ad568 Compare November 30, 2021 08:21

github-actions bot deployed to Preview November 30, 2021 08:21 View deployment

github-actions bot deployed to Preview December 1, 2021 11:44 View deployment

github-actions bot deployed to Preview December 1, 2021 16:40 View deployment

pksjce force-pushed the pk/button-enhancements branch from e703003 to bbc4eac Compare December 2, 2021 04:55

github-actions bot deployed to Preview December 2, 2021 04:55 View deployment

github-actions bot deployed to Preview December 2, 2021 05:49 View deployment

pksjce force-pushed the pk/button-enhancements branch from 00c4034 to d4244f6 Compare December 8, 2021 08:48

github-actions bot deployed to Preview December 8, 2021 08:48 View deployment

rezrah reviewed Dec 10, 2021

View reviewed changes

rezrah approved these changes Dec 10, 2021

View reviewed changes

github-actions bot deployed to Preview December 13, 2021 06:07 View deployment

pksjce force-pushed the pk/button-enhancements branch from 32a6b3b to d1347f5 Compare December 13, 2021 06:07

github-actions bot deployed to Preview December 13, 2021 06:08 View deployment

github-actions bot deployed to Preview December 13, 2021 10:35 View deployment

pksjce force-pushed the pk/button-enhancements branch from d258935 to 185ba3b Compare December 13, 2021 21:00

github-actions bot deployed to Preview December 13, 2021 21:01 View deployment

pksjce force-pushed the pk/button-enhancements branch from 185ba3b to 3f7fb42 Compare December 13, 2021 22:15

github-actions bot deployed to Preview December 13, 2021 22:15 View deployment

pksjce added 8 commits December 14, 2021 09:15

Create icon only button

8bf2b36

Add ButtonLink and IconButton components

db7e8a3

Lint and test

912f933

Remove react router for now. Maybe add an example later

4230ec8

Fix the typescript error on as prop

fa04a60

Fix lint issue

5ea901b

Fix

ad5f7fb

1. Bug with disabled 2. Tests in testing lib

lint issues

ceb847c

pksjce force-pushed the pk/button-enhancements branch from 3f7fb42 to ceb847c Compare December 13, 2021 22:15

github-actions bot deployed to Preview December 13, 2021 22:15 View deployment

pksjce merged commit 4005d6a into main Dec 13, 2021

pksjce deleted the pk/button-enhancements branch December 13, 2021 22:33

Button: enhancements with ButtonLink and IconButton #1659

Button: enhancements with ButtonLink and IconButton #1659

Uh oh!

Conversation

pksjce commented Nov 26, 2021

Merge checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

changeset-bot bot commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actions bot commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

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!

rezrah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Nov 26, 2021 •

edited

Loading

github-actions bot commented Nov 26, 2021 •

edited

Loading