Fix issue - semantic props overwritten when single prop used; update semantic to 1.3.1

[jacqueswho]

Reasons for making this change

• Semantic updated to 1.3.1
• getSemanticProps to allow user pass semantic ui props including defaults
• getSemanticErrorProps to allow user to pass ui error props
• fixes this issue

Checklist

• I'm updating documentation
  • I've checked the rendering of the Markdown text I've added
• 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]

Can you update https://github.com/rjsf-team/react-jsonschema-form/blob/master/CHANGELOG.md to have a full list of all changes for this theme?

[epicfaace]

Sorry for the delay! I will take a look soon.

…

-- Ashwin Ramaswami

On Mon, Nov 1, 2021 at 11:02 AM Jacques Nel ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In CHANGELOG.md <#2590 (comment)> : > @@ -33,6 +33,10 @@ should change the heading of the (upcoming) version to include a major version b ## @rjsf/fluent-ui - fluent-ui: Allow value of 0 in TextWidget (#2497) +## @rjsf/semantic-ui +- "semantic-ui-react" updated to v1.2.1 (#2590) +- fixed passing semantic theme specific props, by overiding defaults, with your own semantic props (#2590) @epicfaace <https://github.com/epicfaace> is there anything else — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2590 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4MXZYYWLSDIOPI44EIETUJ2TYFANCNFSM5G46LU7Q> .

[epicfaace]

@heath-freenome if you have a minute to help review this PR, that would be great, otherwise no worries

[jacqueswho]

@heath-freenome thanks for the feedback. made the changes

[heath-freenome]

@epicfaace Looks good to me

[heath-freenome]

I actually made the suggested change in my Mui 5 PR so I know it works... But it is up to you

[epicfaace]

Thanks for the review @heath-freenome ! I think the only thing remaining is that I'd like to have a clearer description in the changelog to describe what issue exactly was fixed (rather than just linking to this PR).

[jacqueswho]

@epicfaace I'm not sure what else you want me to describe, could you make let know how you want it written

[epicfaace]

The changelog right now says "fixed passing semantic theme specific props, by only overiding defaults that match, with your own semantic props (#2590)". That's confusing for a user who is seeing this changelog, and I'm not quite sure what it means either. Maybe we can add more sentences / an example to describe what it's about? I remember you said earlier that "The problem was that if you passed one specific theme prop of your own all other default props were lost."

This is actually why making an issue on GitHub Issues and linking it to this PR will make this much easier. It will then be really clear what exactly the issue is, and we can easily then convert the issue to a changelog description item. Do you think you'd be able to do that?

[jacqueswho]

@epicfaace thanks, I will create an issue and link it to this PR. But after this is merged in I think we should move this into core util to obtain theme specific props. Question though, should the default theme specific props be over ridden if a user passes his own. for example these are the default props for a selectWidget

{
      inverted: "false",
      selection: true,
      fluid: true,
      scrolling: true,
      upward: false,
    }

should a user now pass

{
  'ui:options' : {
     semantic: {
       fluid: false,
     }
   }
}

that select dropdown is going to look terrible because it has the lost the rest of the default props.

so should the new props be

{
      inverted: "false",
      selection: true,
      fluid: false,
      scrolling: true,
      upward: false,
    }

OR

{
      fluid: false,
 }

So leave the default props in place unless that prop is passed exclusively by the user

[epicfaace]

Are there any breaking changes in this version bump that would make it different to use @rjsf/semantic-ui? If so, we might need to bump the rjsf version to a major version as well.

[epicfaace]

Thanks for making the issue @jacqueswho! I think this is the only remaining outstanding issue before we can get this merged.

[jacqueswho]

@epicfaace according to this no.

[jacqueswho]

@epicfaace I have updated the getSemanticProps function, to make it more simpler

[epicfaace]

What does defaultSchemaProps && defaultSchemaProps do? Why not just do defaultSchemaProps?

[jacqueswho]

if it is undefined it wont destructure the props. Its quite a neat approach for null/ undefined checks

[epicfaace]

Thanks! Sorry it took so long, the issue helped to clarify what's it about.

[epicfaace]

Suggested change

	- fixed passing [semantic theme specific props](https://react-jsonschema-form.readthedocs.io/en/latest/api-reference/themes/semantic-ui/uiSchema/), by only overiding defaults that match, with your own semantic props (https://github.com/rjsf-team/react-jsonschema-form/pull/2590) related to this [issue 2619](https://github.com/rjsf-team/react-jsonschema-form/issues/2619)
	- fixed an issue where all semantic props overwritten when a single [semantic theme-specific prop](https://react-jsonschema-form.readthedocs.io/en/latest/api-reference/themes/semantic-ui/uiSchema/) is passed in ([issue 2619](https://github.com/rjsf-team/react-jsonschema-form/issues/2619)) (https://github.com/rjsf-team/react-jsonschema-form/pull/2590)

[epicfaace]

Actually, just one more thing. Can you add a snapshot test where we add a single semantic-specific prop to override an input field, then render a <Form> element, then the snapshot should ensure that the form is not overridden? This way we test the full end-to-end use case for #2619.

[jacqueswho]

@epicfaace test updated