-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
feat: required labels on listings management fields #1924
Conversation
✔️ Deploy Preview for dev-storybook-bloom ready! 🔨 Explore the source changes: 07526c7 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/615fb40e9a82a300085ac981 😎 Browse the preview: https://deploy-preview-1924--dev-storybook-bloom.netlify.app |
❌ Deploy Preview for dev-bloom failed. 🔨 Explore the source changes: 07526c7 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/615fb40d04b1e10009e8984d |
✔️ Deploy Preview for dev-partners-bloom ready! 🔨 Explore the source changes: 07526c7 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/615fb40ded839d0007f1308b 😎 Browse the preview: https://deploy-preview-1924--dev-partners-bloom.netlify.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are accessibility issues in these changes.
ui-components/src/forms/Select.tsx
Outdated
{label} | ||
</label> | ||
<div className={controlClassName}> | ||
<select |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 You fixed the issue(s)! Great work.
dac0e8c
to
e82d29b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 You fixed the issue(s)! Great work.
Can we:
|
@slowbot sure thing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 You fixed the issue(s)! Great work.
@emilyjablonski Can we add a label to the default state of photos and units similar to the mockups so we can associate error messages to the labels? |
Updated! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 You fixed the issue(s)! Great work.
54890de
to
07526c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 You fixed the issue(s)! Great work.
Pull Request Template
Issue
Addresses #1580
Description
Add required notes to the required fields in listings management. Had to add a few optional props to the field components.
Type of change
How Can This Be Tested/Reviewed?
Open listings management and ensure before you've submitted that it looks good and then try to publish without entering anything and ensure the notes still look good when you also have error messages.
Checklist:
yarn generate:client
if I made backend changes