Bug Fix empty html useless added by PR #1123 / v1.2.0

[pieplu]

Reasons for making this change

The merged PR #1123 add useless wrapping html tag (with empty className) for all field, even if they are no additonal field. This PR fix this.
See comment: #1123 (comment)

Some points:

• I add new cssClass form-additional-* to help targeting (like for array field)
• I use col-9 and 3, like array field, and I don't put on the same line the "Key" field and the "value" field by default. It's unusable on small screen. I think it will be the job of the integrator to add css rules to put this on the same line, if needed.
• Maybe it will be good a day to found an other solution for the "key" label name, its really not internationalizable...

Checklist

• I'm adding or updating code
  • I've added and/or updated tests
  • I've updated docs if needed
  • I've run npm run cs-format on my branch to conform my code to prettier coding style

[pieplu]

@aerfio are you agree with this change?

[aerfio]

@pieplu I'm just a contributor, not a codeowner, so you don't have to ask for my agreement 😛

But personally I think delete button shouldn't end in 4/5 of width, it would be nice if that blank space on the right side was filled

I don't know if you can do what I proposed in bootstrap3, but you can certainly do that using flex; I don't know what is the final stance of codeowners considering this matter - it is used here btw: https://github.com/mozilla-services/react-jsonschema-form/blob/174e136af4fe728eb79e92594733ea729f3d659f/src/components/fields/ArrayField.js#L59

[pieplu]

@aerfio It's because it's a change on your contribution.

For the blank space, I think it's not a real problem. It's not the best visual, but we are limited on bootstrap style.
Dev's can esealy overwrite css to style this.
I thinks it's better than apply a style on a DOM node, because it's realy harder to overwrite, I agree with @glasserc position. On the ArrayField I think it's a technical depth.

Maybe a good option for inline-style, will be to have an api to overwrite the inline-style object, like react-select do.

[epicfaace]

@pieplu Could you please modify this PR not to use the WrapIfAdditional component? That way it will be easier to see what logic exactly you are changing. Thanks!

[pieplu]

I push it, but I don't think it will be better. It will insert code duplication. It's a big risk of bugs for all future editing of the component.

The reason why I add WrapIfAdditionals Component is to be more understandable. But maybe I lost it.

I will try to explain it:

SchemaFields.js part, before the "additional properties feature":

<div className={classNames}>
     {displayLabel && <Label label={label} required={required} id={id} />}	
      {displayLabel && description ? description : null}	
      {children}	
      {errors}	
      {help}	
  </div>

Html output:

PR #1021 and #1123 added some code around that to support the additional properties feature.
The big problem, since v1.2.0, for ALL fields (even if no additional properties), looks like this:

To be able to support additional properties feature, it will be important to wrap the schema field with some div. It's the problem, the level of complexity had grown up, and if we preserved same render for all other fields, the wrapping div should not be present any time.

Maybe a better solution will be to introduce a new field component (like ArrayFieldDo) ?

[epicfaace]

@pieplu , thanks for the explanation, I see what you're doing now. To remove code duplication, you could store the duplicated code in a variable as shown here: https://stackoverflow.com/a/33710901

Also, can you please include only the changes that fix the wrapping div's and the col-xs-12 div in this PR? I appreciate the other changes you made, but I think it's best if those are made on another PR so there is a chance to examine and discuss those changes separately. 👍

[pieplu]

@epicfaace Good point for the other PR. +1
For your suggestion, I will, try, but I don't think it will works, because it's not one React node, but many. And on React 15 we don't have React.Fragment built-in.

[epicfaace]

@pieplu sounds good, please make that other PR!

And, yes, I see your point for using WrapIfAdditional now. Once you separate the additional changes made to the other PR, then the diff will be cleaner even with the WrapIfAdditional component. Thanks for sticking with this! 😄

[epicfaace]

@pieplu just a reminder to make the PR when you get a chance! I'd like to start merging your changes.

[pieplu]

I keep it in mind, sorry, I will try to take time this week.

…

On févr. 25 2019, at 5:21 pm, Ashwin Ramaswami ***@***.***> wrote: @pieplu (https://github.com/pieplu) just a reminder to make the PR when you get a chance! I'd like to start merging your changes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (#1158 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/ADG_fSaK2-wLBDUI1XNCZCQ6vFDV-1dfks5vRGHygaJpZM4aWR62).

[pieplu]

@epicfaace done!

I will make an other PR with CSS className change for easy customisation

[epicfaace]

One more thing. I believe you don't need parentProps, so the way I suggested might be cleaner.

[pieplu]

@epicfaace Done!

[pieplu]

@epicfaace re Done!

[epicfaace]

Great! It looks good. Thanks for the good work!