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.

Sign up for GitHub

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

Jump to bottom

Add delete button for additionalProperties key-value pair #1123

Merged

glasserc merged 15 commits into rjsf-team:master from aerfio:delete-button

Jan 10, 2019

Contributor

aerfio commented Jan 4, 2019 •

edited

Loading

Reasons for making this change

This PR adds delete button for key-value pair in additionalProperties, also rearanges those two inputs + button to be in line, not one below another. It also removes the ability to remove pair by removing content of value input - we have button for that.

I have also fixed tests related to time widget (change from 2018 to 2019 made two tests crash).

Work largely based on this PR: #1085, but since nothing has changed there for some time I decided to make this PR.

This PR would resolve #1099.

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
- If I'm adding a new section, I've updated the Table of Content
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
I'm adding a new feature
- I've updated the playground with an example use of the feature

Contributor Author

aerfio commented Jan 4, 2019

@edi9999 @glasserc Can you have a look at this?

Mateusz Puczynski added 11 commits

January 7, 2019 16:09


          add del button functionality

b59a9eb


          delete unnecesary console.logs

a748ee1


          fix tests

6559d1d


          remove unnecesary console.log

8f5f597


          Fix disappearing key-value pair and repair delete button

ba1181d


          Add .vscode to gitignore

939209c


          fix tests

d4569a9


          Make test work for select/radio button

eb72a4e


          Add test cases

759d338


          Remove one file from PR and one helper function

c4719af


          Add one classname for value input

d21e0aa

aerfio force-pushed the delete-button branch from 5f73233 to d21e0aa Compare

January 7, 2019 15:10

Mateusz Puczynski added 2 commits

January 7, 2019 16:13


          remove accidentally added .vscode

7f1a7f4


          linted codebase

a558246

glasserc reviewed

View reviewed changes

Contributor

glasserc left a comment

Thanks for the effort. I'd like to merge this to clear out the old PR and issue.

I'm a bit concerned about the change of behavior for setting a field value to an empty string. I understand the rationale here but we've had some problems with this empty-form-input => empty string / undefined conversion before. See #289, #539. Introducing different behavior here for additional properties seems like an accident waiting to happen. Is there another approach that could get us what we want?

.gitignore Outdated

@@ @@ -4,3 +4,4 @@ build @@
               dist
               lib
               yarn.lock
+              .vscode

Contributor

glasserc Jan 9, 2019

I don't think this belongs here. It would be better to put this in your .git/info/exclude.

src/components/fields/SchemaField.js Outdated

+                  paddingRight: 10,
+                  fontWeight: "bold",
+                  height: "34px",
+                };

Contributor

glasserc Jan 9, 2019

This project doesn't use inline styles. You can rely on Bootstrap styles.

src/components/fields/ObjectField.js Show resolved Hide resolved

src/components/fields/ObjectField.js Outdated

@@ @@ -79,9 +80,21 @@ class ObjectField extends Component { @@
                   );
                 }
-                onPropertyChange = name => {
+                onPropertyChange = (name, additionalProperties = false) => {

Contributor

glasserc Jan 9, 2019

How about naming this parameter isAdditionalProperty or addedByAdditionalProperties?

src/components/fields/ObjectField.js Outdated

@@ @@ -93,6 +106,18 @@ class ObjectField extends Component { @@
                   };
                 };
+                onDropIndexClick = index => {
+                  return event => {
+                    if (event) {

Contributor

glasserc Jan 9, 2019

I don't know if we need to check whether event exists or not..

src/components/fields/ObjectField.js Outdated

@@ @@ -200,6 +224,9 @@ class ObjectField extends Component { @@
                     TitleField,
                     DescriptionField,
                     properties: orderedProperties.map(name => {
+                      const additionalPropertyFlag = schema.properties[name].hasOwnProperty(

Contributor

glasserc Jan 9, 2019

I'd like to rename this variable too.

src/components/fields/ObjectField.js Outdated

@@ @@ -93,6 +106,18 @@ class ObjectField extends Component { @@
                   };
                 };
+                onDropIndexClick = index => {

Contributor

glasserc Jan 9, 2019

I think the name index in the names of the prop, the function, and the argument are misleading. This should be a string property name, right?


          remove inline styles, make use of bootstrap 3 styles and fix related …

967e319

…tests

aerfio force-pushed the delete-button branch from 534122d to 967e319 Compare

January 10, 2019 13:10

Contributor Author

aerfio commented Jan 10, 2019

@glasserc Existence of addedByAdditionalProperties flag feels a bit hacky, but it's used only in additionalProperties, and without it emptying a value input makes pair disappear, which is counter-intuitive -> we have a delete button introduced in this PR for this.

If emptying a string would not make pair disappear it would be great, but I couldn't find a better place for it - if you know a better place/method how to make it work like it should then please let me know 👍 .

After this PR gets merged when can we expect next release?


          revert changes to .gitignore

73cde14

glasserc merged commit aabca71 into rjsf-team:master

Contributor

glasserc commented Jan 10, 2019

Thanks for your contribution. I agree that this is maybe the least bad of the possible solutions to clearing out an input field. I'll cut a release later today.

glasserc mentioned this pull request

multiple bugs with additionalProperties component #1099

Closed

2 tasks

glasserc pushed a commit that referenced this pull request


          Add delete button for additionalProperties key-value pair (#1123)

* add del button functionality

* delete unnecesary console.logs

* fix tests

* remove unnecesary console.log

* Fix disappearing key-value pair and repair delete button

* Add .vscode to gitignore

* fix tests

* Make test work for select/radio button

* Add test cases

* Remove one file from PR and one helper function

* Add one classname for value input

* remove accidentally added .vscode

* linted codebase

* remove inline styles, make use of bootstrap 3 styles and fix related tests

* revert changes to .gitignore

glasserc mentioned this pull request

Fixed support for "additionalProperties" #1085

Closed

4 tasks

aerfio deleted the delete-button branch

January 10, 2019 19:43

pieplu reviewed

View reviewed changes

src/components/fields/SchemaField.js

-                    {children}
-                    {errors}
-                    {help}
+                    </div>

Contributor

pieplu Jan 18, 2019 •

edited

Loading

This code add useless empty div (and a empty col-2) for all fields

Before:

After:

I think it's a regression. In our case, its break some styles

Contributor Author

aerfio Jan 18, 2019

@pieplu I agree that lines 134 and 135 of this file can surely be merged into one (mistake on my side), which would probably leave just one empty div which is in line 150, but I at the first glance I don't know how to achieve same appearence using Bootstrap 3, and have a look at this: #1123 (comment)

Contributor

pieplu Jan 18, 2019

Ok, I can try PR a fix with bootstrap style

Contributor

pieplu commented Jan 18, 2019 •

edited

Loading

Hi! @aerfio , as I say on this comment: https://github.com/mozilla-services/react-jsonschema-form/pull/1123/files#r249176646 , this PR include useless div, and break some styles.

pieplu mentioned this pull request

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

Merged

4 tasks

epicfaace pushed a commit that referenced this pull request


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

10a6b64

* Infer type from enum if a type is not provided to SelectWidget (#1100)

Connects to #1098

Change-type: minor
Signed-off-by: Lucian <lucian.buzzo@gmail.com>

* No more useless div on schema field not additional

* Fix tests (remove useles html)
- Revert some test added by
https://github.com/mozilla-services/react-jsonschema-form/pull/1123/files
- use new css class for form-additional insted layout css class
for test selection

* Don't use WrapIfAdditonal Component to hav a clerer diff, but adding code duplicate

* Use a WrapIfAdditional Component to have a cleanner code / diff

* Fix tests

* Ignore vscode Ide folder

* Remove useless parentProps

* Fix old test selector for form additional

CodeGains pushed a commit to CodeGains/react-jsonschema-form that referenced this pull request


          Bug Fix empty html useless added by PR rjsf-team#1123 / v1.2.0 (rjsf-…

a432960

…team#1158)

* Infer type from enum if a type is not provided to SelectWidget (rjsf-team#1100)

Connects to rjsf-team#1098

Change-type: minor
Signed-off-by: Lucian <lucian.buzzo@gmail.com>

* No more useless div on schema field not additional

* Fix tests (remove useles html)
- Revert some test added by
https://github.com/mozilla-services/react-jsonschema-form/pull/1123/files
- use new css class for form-additional insted layout css class
for test selection

* Don't use WrapIfAdditonal Component to hav a clerer diff, but adding code duplicate

* Use a WrapIfAdditional Component to have a cleanner code / diff

* Fix tests

* Ignore vscode Ide folder

* Remove useless parentProps

* Fix old test selector for form additional

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet