Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Let aria-required be undefined if not passed #4058

Merged
merged 4 commits into from
Dec 14, 2023
Merged

Let aria-required be undefined if not passed #4058

merged 4 commits into from
Dec 14, 2023

Conversation

mperrotti
Copy link
Contributor

Follow up to #4023

Does not pass aria-required attribute to input or textarea if it is undefined. This fixes some tests that were breaking in dotcom.

Changelog

New

Changed

Before:
aria-required is set to "false" if the required prop is not passed to TextInput or Textarea .

After:
aria-required is not set at all if the required prop is not passed to TextInput or Textarea .

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@mperrotti mperrotti requested review from a team and broccolinisoup December 13, 2023 21:34
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 13, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 5888cfb

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

mperrotti
mperrotti commented Dec 13, 2023
View reviewed changes
src/TextInput/TextInput.tsx
@@ -144,7 +144,7 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputProps>(
onFocus={handleInputFocus}
onBlur={handleInputBlur}
type={type}
aria-required={required ? 'true' : 'false'}
aria-required={required}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still use aria-required={required ? 'true' : 'false'} on Radio and Checkbox. Should we update those as well? Or are we worried this could also break tests in dotcom?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the broken tests?? 👀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some tests in dotcom that compare the dom, so any additional props even when false fail CI 🤦

Ideally they should not depend on attributes from within primer components, but it's common :(

Copy link
Member

@siddharthkp siddharthkp Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or are we worried this could also break tests in dotcom?

Only one way to find out, going to run the integration tests for this PR

Update: Seems like there are no new errors 🎉 https://github.com/github/github/actions/runs/7208501227/job/19637623263

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 13, 2023 •
edited
Loading

size-limit report 📦

Path Size
dist/browser.esm.js 105.54 KB (0%)
dist/browser.umd.js 106.13 KB (-0.01% 🔽)

@github-actions github-actions bot temporarily deployed to storybook-preview-4058 December 13, 2023 21:40 Inactive
broccolinisoup
broccolinisoup reviewed Dec 14, 2023
View reviewed changes
src/TextInput/TextInput.tsx
@@ -144,7 +144,7 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputProps>(
onFocus={handleInputFocus}
onBlur={handleInputBlur}
type={type}
aria-required={required ? 'true' : 'false'}
aria-required={required}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick question do we want to have aria-required="false" if required is passed but as false? (Example usage)

Or do we only want to have aria-required when it is only true?

Copy link
Member

@siddharthkp siddharthkp Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do we only want to have aria-required when it is only true?

yes :)

I asked Mike to create this PR because I found some snapshot tests in dotcom that compare the dom, so any additional props, even when false fail integration tests 🤦

src/TextInput/TextInput.tsx
@@ -144,7 +144,7 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputProps>(
onFocus={handleInputFocus}
onBlur={handleInputBlur}
type={type}
aria-required={required ? 'true' : 'false'}
aria-required={required}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the broken tests?? 👀

siddharthkp
siddharthkp approved these changes Dec 14, 2023
View reviewed changes
Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@siddharthkp siddharthkp added the patch release bug fixes, docs, housekeeping label Dec 14, 2023
@siddharthkp siddharthkp added this pull request to the merge queue Dec 14, 2023
Merged via the queue into main with commit 4d841b7 Dec 14, 2023
32 checks passed
@siddharthkp siddharthkp deleted the mp/aria-required-undefined branch December 14, 2023 12:32
@primer primer bot mentioned this pull request Dec 14, 2023
Merged
@siddharthkp siddharthkp mentioned this pull request Dec 14, 2023
Closed
4 tasks
@primer primer bot mentioned this pull request Dec 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@broccolinisoup broccolinisoup broccolinisoup left review comments

@siddharthkp siddharthkp siddharthkp approved these changes

Assignees

@mperrotti mperrotti

Labels
patch release bug fixes, docs, housekeeping
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mperrotti @broccolinisoup @siddharthkp