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

Allows for a custom tagName to be used in place of "form" #1345

Merged
merged 1 commit into from Jul 9, 2019

Conversation

juliankigwana
Copy link
Contributor

@juliankigwana juliankigwana commented Jul 5, 2019

Reasons for making this change

Sometimes we need a way to nest the RJSF components so that we can utilise the rendering capabilities within our components. For example, when rendering a custom Array<Object> component, the child properties can be rendered in a child form. However doing this will produce invalid html, a nasty error in the console, and possibly cause unforeseen problems across browsers.

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

@epicfaace
Copy link
Member

epicfaace commented Jul 6, 2019

Looks great, thanks! Can you add a test for it?

I'm also wondering if not having a form tag will break anything (such as event handlers) -- nothing comes to mind, but let me know if you think of something.

@juliankigwana
Copy link
Contributor Author

I added a test. I cannot think of any obvious other than the default native browser behaviour might not work. But I would imagine that if you were manually changing the tagName, then you would be aware of this.

@epicfaace
Copy link
Member

@juliankigwana that makes sense. Can you add that bit as a warning in the documentation? (that the native browser behavior might not work)

@epicfaace
Copy link
Member

Additionally, the only allowed values for tagName will be valid JSX HTML tags, correct? Can you add a test in which one tries an invalid tagName?

@juliankigwana
Copy link
Contributor Author

juliankigwana commented Jul 8, 2019

Additionally, the only allowed values for tagName will be valid JSX HTML tags, correct? Can you add a test in which one tries an invalid tagName?

Screen Shot 2019-07-08 at 11 09 11

I'm not sure we can test for it. Most browsers will render an unknown banana tag as a block element. It would be up to the developer to know what is valid HTML.

I shall update the documentation with the warning and something saying it expects a valid HTML tag.

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.

Makes sense, thanks! Can you apply this suggestion -- and then we'll be good to merge this.

@juliankigwana
Copy link
Contributor Author

Thanks @epicfaace. I did apply the suggestion, but it's still showing here as changes requested. Do I need to do anything else?

@epicfaace
Copy link
Member

Nope, looks good -- thanks!

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.

None yet

2 participants