Skip to content

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

Merged
epicfaace merged 1 commit intorjsf-team:masterfrom
juliankigwana:enable-custom-tagname
Jul 9, 2019
Merged

Allows for a custom tagName to be used in place of "form"#1345
epicfaace merged 1 commit intorjsf-team:masterfrom
juliankigwana:enable-custom-tagname

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 juliankigwana force-pushed the enable-custom-tagname branch from 7c1fddc to 1686d8b Compare July 7, 2019 19:52
@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.

@juliankigwana juliankigwana force-pushed the enable-custom-tagname branch from 1686d8b to 630b47d Compare July 8, 2019 10:38
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 juliankigwana force-pushed the enable-custom-tagname branch from 630b47d to ade90c4 Compare July 8, 2019 15:53
@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.

2 participants