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.

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

Help elements need a predictable ID #2360

Merged
merged 3 commits into from
May 19, 2021
Merged

Conversation

bradhugh
Copy link
Contributor

@bradhugh bradhugh commented May 4, 2021

Reasons for making this change

When a ui:help is added to uiSchema for a property, the generated p or div tag needs to have an id attribute so it addressable. This same thing already exists for description fields. This is needed for association with the corresponding form fields for accessibility (like adding aria-describedby="helpId") on the associated input fields.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

Deploy preview

Once CI finishes, you should be able to view a deploy preview of the playground from this PR at this link: https://deploy-preview-[PR_NUMBER]--rjsf.netlify.app/

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Thanks -- are there any similar changes that could / should be done for other themes' help elements as well?

@epicfaace epicfaace self-assigned this May 4, 2021
@bradhugh
Copy link
Contributor Author

bradhugh commented May 4, 2021

Thanks -- are there any similar changes that could / should be done for other themes' help elements as well?

It doesn't look like many of the templates actually override the Help field. I don't know anything about semantic-ui, but take a look at FieldTemplate.js in semantic-ui. Right now it's got the following:
<HelpField helpText={rawHelp} id={id} />

I'll try to dig a bit and understand where it's getting "id" from in this context.

@bradhugh
Copy link
Contributor Author

bradhugh commented May 4, 2021

Yep, that's what I thought. It's actually duplicating the id here which is a bug. I'll make a change to fix this bug while I'm here.

image

@bradhugh
Copy link
Contributor Author

@epicfaace anything else I need to do here?

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Thanks! This looks great.

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Actually, maybe we should add some snapshot tests for the themes with help fields? Just so we make sure that in the future, help fields always have ids that end with "__help" and this doesn't regress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants