Refine 'as' prop type in button component and fix name of aria type #2659

kendallgassner · 2022-12-06T20:50:14Z

Describe your changes here.

aria-labelby -> aria-labelledby (aria-labelby is not an aria attribute)
restrict the button 'as' prop at runtime to be either a semantic <a> or <button>

I introduced the library react-merge-refs to do this but please let me know if there is a better way 😸

Open Questions

Should this be a patch? Since its a new warning message?

Screenshots

Please provide before/after screenshots for any visual changes

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.

changeset-bot · 2022-12-06T20:50:17Z

🦋 Changeset detected

Latest commit: 7f32cfc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

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

github-actions · 2022-12-06T20:59:23Z

size-limit report 📦

Path	Size
dist/browser.esm.js	82.8 KB (+0.06% 🔺)
dist/browser.umd.js	83.45 KB (+0.06% 🔺)

joshblack

Looks great! Thanks for doing this 🙏

Just left one comment, I think it might be the convention in PRC but I'm not 100% sure 😅

I think all that's left after the comment is to add a changeset and should be good to go 👍

kendallgassner · 2022-12-06T22:39:03Z

@joshblack

Just left one comment, I think it might be the convention in PRC but I'm not 100% sure 😅

Did you submit the comment? 😅 I am not seeing it on my end.

Also, would this be a patch?

joshblack · 2022-12-07T17:21:15Z

src/Button/ButtonBase.tsx

@@ -18,6 +19,7 @@ const trailingIconStyles = {
 const ButtonBase = forwardRef<HTMLElement, ButtonProps>(
  ({children, as: Component = 'button', sx: sxProp = defaultSxProp, ...props}, forwardedRef): JSX.Element => {
    const {leadingIcon: LeadingIcon, trailingIcon: TrailingIcon, variant = 'default', size = 'medium', ...rest} = props
+    const innerRef = React.useRef<HTMLElement>()


Suggested change

const innerRef = React.useRef<HTMLElement>()

const innerRef = React.useRef<HTMLElement>()

useRefObjectAsForwardedRef(forwardedRef, innerRef)

Here was that convention that I was talking about! This should be available in src/hooks/useRefObjectAsForwardedRef.ts. This seems to show up in a couple components that both have a local and forwardedRef

AMAZING thank you!

joshblack · 2022-12-07T17:22:49Z

@kendallgassner sorry about that! Just left a comment above 👍 And good question about patch versus minor, I think regardless it will be included in the next minor so what you have currently in the changeset should totally work.

src/Button/ButtonBase.tsx

Co-authored-by: Josh Black <joshblack@github.com>

…rop-type

…om/primer/react into kendallg/limit-button-as-prop-type

joshblack

Thanks for doing this!

mattcosta7 · 2022-12-08T16:42:11Z

src/Button/ButtonBase.tsx

+    React.useEffect(() => {
+      if (!(innerRef.current instanceof HTMLButtonElement) && !(innerRef.current instanceof HTMLAnchorElement)) {
+        if (__DEV__) {
+          // eslint-disable-next-line no-console
+          console.warn('This component should be an instanceof a semantic button or anchor')
+        }
+      }
+    }, [innerRef])
+


realize this might be a little late, but should we wrap the entire effect in

if (DEV) {}

I realize that'll trip the linter, but since it's not actually conditional but instead a build time flag, I think that's worth the lint disable, which would strip a few extra bytes from production builds, and cause no actual runtime issues?

Otherwise, we'll end up still doing instanceof checks here, for no reason on every mount?

I opened a PR for this suggestion - #2666

Refine as prop in button component and fix aria type

c791a50

kendallgassner requested review from a team and broccolinisoup December 6, 2022 20:50

kendallgassner requested a review from colebemis December 6, 2022 20:50

kendallgassner temporarily deployed to github-pages December 6, 2022 20:55 Inactive

github-actions bot temporarily deployed to storybook-preview-2659 December 6, 2022 20:56 Inactive

whoops, remove type change

57a6fc6

kendallgassner temporarily deployed to github-pages December 6, 2022 21:02 Inactive

github-actions bot temporarily deployed to storybook-preview-2659 December 6, 2022 21:02 Inactive

fix lint bugs

91e89e9

kendallgassner temporarily deployed to github-pages December 6, 2022 21:13 Inactive

github-actions bot temporarily deployed to storybook-preview-2659 December 6, 2022 21:13 Inactive

joshblack added the 💓collab a vibrant hub of collaboration label Dec 6, 2022

joshblack reviewed Dec 6, 2022

View reviewed changes

kendallgassner added the a11y-eng-secondary label Dec 6, 2022

changeset

e8036cb

kendallgassner temporarily deployed to github-pages December 6, 2022 23:25 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-2659 December 6, 2022 23:26 Inactive

joshblack reviewed Dec 7, 2022

View reviewed changes

kendallgassner added 2 commits December 7, 2022 09:50

use local mergeRef

d28127f

undo package-lock changes

d4ab06e

kendallgassner commented Dec 7, 2022

View reviewed changes

src/Button/ButtonBase.tsx Outdated Show resolved Hide resolved

Update src/Button/ButtonBase.tsx

cc8e2d0

github-actions bot temporarily deployed to storybook-preview-2659 December 7, 2022 17:58 Inactive

github-actions bot temporarily deployed to storybook-preview-2659 December 7, 2022 18:01 Inactive

kendallgassner temporarily deployed to github-pages December 7, 2022 18:02 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-2659 December 7, 2022 18:02 Inactive

joshblack requested changes Dec 7, 2022

View reviewed changes

src/Button/ButtonBase.tsx Outdated Show resolved Hide resolved

Update src/Button/ButtonBase.tsx

a0da738

Co-authored-by: Josh Black <joshblack@github.com>

kendallgassner temporarily deployed to github-pages December 7, 2022 18:43 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-2659 December 7, 2022 18:44 Inactive

kendallgassner added 3 commits December 7, 2022 10:52

Merge https://github.com/primer/react into kendallg/limit-button-as-p…

a2eec8f

…rop-type

Merge branch 'kendallg/limit-button-as-prop-type' of https://github.c…

e4c1103

…om/primer/react into kendallg/limit-button-as-prop-type

fix eslint

17e6150

kendallgassner temporarily deployed to github-pages December 7, 2022 18:59 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-2659 December 7, 2022 18:59 Inactive

define dev

7f32cfc

kendallgassner temporarily deployed to github-pages December 7, 2022 19:17 — with GitHub Actions Inactive

github-actions bot deployed to storybook-preview-2659 December 7, 2022 19:17 View deployment

kendallgassner requested a review from joshblack December 7, 2022 21:08

joshblack approved these changes Dec 7, 2022

View reviewed changes

joshblack merged commit 84d2997 into main Dec 7, 2022

joshblack deleted the kendallg/limit-button-as-prop-type branch December 7, 2022 21:10

primer-css mentioned this pull request Dec 7, 2022

Release tracking #2648

Merged

mattcosta7 reviewed Dec 8, 2022

View reviewed changes

This was referenced Dec 8, 2022

make button as check strip out completely in production builds #2666

Merged

Update types for IconButton, ActionMenu.Button, TabNav.Link and internal consumers #2677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine 'as' prop type in button component and fix name of aria type #2659

Refine 'as' prop type in button component and fix name of aria type #2659

kendallgassner commented Dec 6, 2022 •

edited

Loading

changeset-bot bot commented Dec 6, 2022 •

edited

Loading

github-actions bot commented Dec 6, 2022 •

edited

Loading

joshblack left a comment •

edited

Loading

kendallgassner commented Dec 6, 2022

joshblack Dec 7, 2022

kendallgassner Dec 7, 2022

joshblack commented Dec 7, 2022

joshblack left a comment

mattcosta7 Dec 8, 2022 •

edited

Loading

mattcosta7 Dec 8, 2022

	const innerRef = React.useRef<HTMLElement>()
	const innerRef = React.useRef<HTMLElement>()
	useRefObjectAsForwardedRef(forwardedRef, innerRef)

Refine 'as' prop type in button component and fix name of aria type #2659

Refine 'as' prop type in button component and fix name of aria type #2659

Conversation

kendallgassner commented Dec 6, 2022 • edited Loading

Open Questions

Screenshots

Merge checklist

changeset-bot bot commented Dec 6, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Dec 6, 2022 • edited Loading

size-limit report 📦

joshblack left a comment • edited Loading

Choose a reason for hiding this comment

kendallgassner commented Dec 6, 2022

joshblack Dec 7, 2022

Choose a reason for hiding this comment

kendallgassner Dec 7, 2022

Choose a reason for hiding this comment

joshblack commented Dec 7, 2022

joshblack left a comment

Choose a reason for hiding this comment

mattcosta7 Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

mattcosta7 Dec 8, 2022

Choose a reason for hiding this comment

kendallgassner commented Dec 6, 2022 •

edited

Loading

changeset-bot bot commented Dec 6, 2022 •

edited

Loading

github-actions bot commented Dec 6, 2022 •

edited

Loading

joshblack left a comment •

edited

Loading

mattcosta7 Dec 8, 2022 •

edited

Loading