Skip to content

Conversation

@edgarmueller
Copy link
Contributor

@edgarmueller edgarmueller commented Feb 8, 2019

Sorry, this change got a bit out-of-hand because of updating the tests. If necessary, I can split it up into multipe commits.

  • Re-structure combinator renderers, so that subschemas are passed to
    sub-sequent JsonForms calls
  • Consistent usage of schema & rootSchema, remove scopedSchema prop
  • Refactor mapStateToFieldProps, it relies on being props being passed down now

@edgarmueller edgarmueller requested a review from sdirix February 11, 2019 08:37
* Re-structure combinator renderers, so that subschemas are passed to
  sub-sequent JsonForms calls
* Fix path of oneOf renderer
* Use title property if available for displaying oneOf subschemas
* Consistent usage of schema & rootSchema, remove scopedSchema prop
* Refactor mapStateToFieldProps
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 88.321% when pulling 6a5935c on edgarmueller:fix/one-of-issues into 8b04d3c on eclipsesource:master.

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

The code looks good to me. Because of the size of the change I wasn't able to look at each changed line specifically, but the overall architecture change looks fine to me. I also run the material and angular-material examples and everything looked fine to me.

Some more work is needed, for example for the one-of titles and customizing of the all-of renderer, but these are definitely follow up issues.


/**
* Resolve the given schema path in order to obtain a subschema.
* @param {JsonSchema} schema the root schema from which to start
Copy link
Member

Choose a reason for hiding this comment

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

the (sub-)schema from which to start

@edgarmueller edgarmueller merged commit c7dd597 into eclipsesource:master Feb 13, 2019
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.

3 participants