Skip to content

Conversation

LauraAubin
Copy link
Contributor

Part of #795

WHY are these changes introduced?

If the text field does not have a prefix or suffix, then it doesn't have an aria-labelledby value. Since label is a required prop on TextField, we will always have an id to link to.

WHAT is this pull request doing?

Removing the check for an empty array since the labelledby property should have a value regardless if the TextField has a prefix or suffix.

How to 🎩

  • Open up Storybook
  • Go to the "Modal with primary action" example
  • Inspect the modal's TextField
  • The TextField should have an aria-labelledby matching the label element.

image

  • Notice the label's id PolarisTextField2Label on the input.

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2019

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified3
Files potentially affected9

Details

All files potentially affected (total: 9)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/TextField/TextField.tsx (total: 9)

Files potentially affected (total: 9)

🧩 src/components/TextField/tests/TextField.test.tsx (total: 0)

Files potentially affected (total: 0)


This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

@LauraAubin LauraAubin requested a review from chloerice November 5, 2019 19:14
@LauraAubin LauraAubin force-pushed the text-field-missing-label branch from cb717e4 to 52035f0 Compare November 5, 2019 19:15
@dleroux
Copy link
Contributor

dleroux commented Nov 6, 2019

🎩 works great! Just thinking we should probably add a test for this. Coverage is fine but this is not covered. What do you think?

@LauraAubin
Copy link
Contributor Author

LauraAubin commented Nov 7, 2019

Probably best to add a test so that this fix doesn't get accidentally changed in the future.

Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

🎉 but probably can add a changelog for this?

@dleroux dleroux self-requested a review November 11, 2019 14:27
@LauraAubin LauraAubin force-pushed the text-field-missing-label branch from 4635538 to 391e936 Compare November 12, 2019 16:45
Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

🎉

@LauraAubin LauraAubin merged commit a634929 into master Nov 12, 2019
@LauraAubin LauraAubin deleted the text-field-missing-label branch November 12, 2019 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants