Skip to content

ArrayControl - Delete Button - Vanilla Renderer #2008

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.

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

Closed
rsoika opened this issue Aug 21, 2022 · 10 comments
Closed

ArrayControl - Delete Button - Vanilla Renderer #2008

rsoika opened this issue Aug 21, 2022 · 10 comments
Milestone

Comments

@rsoika
Copy link
Contributor

rsoika commented Aug 21, 2022

Is your feature request related to a problem? Please describe.

I am trying now implement the delete button functionality for the ArrayControl in the Vanilla Renderer. But this seems to be more difficult as expected.

Describe the solution you'd like

My most naive approach was to simple add delete button around the JsonFormsDispatch:

...
            return (
              <div><JsonFormsDispatch
                schema={schema}
                uischema={childUiSchema || uischema}
                path={childPath}
                key={childPath}
                renderers={renderers}
              /><button
              	aria-label={`Delete`}
                onClick={() => {
                  if (window.confirm('Are you sure you wish to delete this item?')) {
                    data.splice(index, 1); 
                  }
                }}>Delete</button></div>
            ); 
...

Technical this works, but the component is not rerendered.

Describe alternatives you've considered

The TableArrayControl instead used an method removeItems wrapped by a method confirmDelete

  confirmDelete = (path: string, index: number) => {
    const p = path.substring(0, path.lastIndexOf(('.')));
    this.props.removeItems(p, [index])();
  };

The button triggers this method:

        <button
          aria-label={`Delete`}
          onClick={() => {
            if (window.confirm('Are you sure you wish to delete this item?')) {
              this.confirmDelete(childPath, index);
            }
          }}
        >
          Delete
        </button>

I tried to use somehow the removeItems method but I did not succeed because of the missing props. At least I did not find out how to build a similar variable in the ArrayControl.

Can you please give me a hint what the correct approach would be so that I can prepare a pull request.

Framework

React

RendererSet

Vanilla

Additional context

No response

@sdirix
Copy link
Member

sdirix commented Aug 21, 2022

Hi @rsoika,

The first approach did not work because the data was modified directly, i.e. data.splice(index, 1);. This is not allowed in React. In React any modification must go through the managed state with its specific updating methods (e.g. setState). As this is bypassed here the component is not rerendered. It also bypasses the JSON Forms state management, so the validation pass will also be missed, etc.

In the JSON Forms mappings we expose the update of the managed state via the handleChange methods. So in this case we need to call handleChange with the path to the array and hand over a new array with the element in question not being a part of it. In the array mappings we conveniently expose this as removeItems. The removeItems is already handed over to the ArrayControl too, you just need to bind it in the ArrayControl props destructor like addItem.

Regarding rendering: I would like to suggest to use a "header" structure like this:

<div className="array.control.element">
  <div className="array.control.element.header">
    <button className="array.control.element.delete> Delete </button>
  </div
  <div className="array.control.element.content">
    <JsonFormsDispatch />
  </div>
</div>

Of course the class name should ideally not be hard coded but derived from the class system.

This structure is more future proof as usually the moveUp, moveDown buttons will be added at some point too. Also a collapse functionality with some naming could then be added in a straightforward manner. You could have a look at the Vue Vanilla templates for inspiration, see here and here.

@rsoika
Copy link
Contributor Author

rsoika commented Aug 22, 2022

Yes, I have already tried the removeItems method but this did not work.

The code looks currently like this:

      <div className={classNames.children}>
        {data ? (
          range(0, data.length).map(index => {
            const childPath = composePaths(path, `${index}`);
            return (
              <div><JsonFormsDispatch
                schema={schema}
                uischema={childUiSchema || uischema}
                path={childPath}
                key={childPath}
                renderers={renderers}
              /><button
              	aria-label={`Delete`}
                onClick={() => {
                	if (window.confirm('Are you sure you wish to delete this item?')) {
                       removeItems(path,[index]);
                    }
                }}>Delete</button></div>
            );
          })
        ) : (
            <p>No data</p>
        )}
      </div>

But removeItems(path,[index]); has no effect

In the TableArrayControl.txs the method is wrapped by the a method call like this:

  confirmDelete = (path: string, index: number) => {
    const p = path.substring(0, path.lastIndexOf(('.')));
    this.props.removeItems(p, [index])();
  };

The question for me is: what is this.props ? In construction of the ArrayControl is very different to the TableArrayContol which is defining a const 'props'. But so far, I am unable to create a similar method call....

@sdirix
Copy link
Member

sdirix commented Aug 22, 2022

In the ArrayControl you could also name the argument props and use props.X everywhere. However as this is just busy work we don't even give props a name but just "split it open" immediatly.

Looking at the code there is a small difference and this is probably why it's not working for you.

In the "current code" it looks like this:

removeItems(path,[index]);

while in the code which is working it looks like this:

this.props.removeItems(p, [index])();

Explanation:

removeItems is a function generator, it returns a function which can be bound against onClick or something else. So in theory one could write: onClick={removeItems(p, [index])}.

Now in the invocations here it's not used like this but wrapped into an own function. Therefore now you need to call the actual function which was generated, i.e. you need to invoke removeItems(path,[index])();

@rsoika
Copy link
Contributor Author

rsoika commented Aug 22, 2022

yes, this is working:

removeItems(path,[index])();

I was not aware that removeItems is a function generator and I have overseen the sufix ();

....As an old Java developer, I fear I will never have the chance to recognize the 'beauty' of this kind of coding... ;-)

I will now prepare my pullRequest....

@rsoika
Copy link
Contributor Author

rsoika commented Aug 22, 2022

sorry, once again:

removeItems works fine. But moveUp or moveDown seems not to work the same way. I got the runntime error

ArrayControl.tsx:105 Uncaught TypeError: moveUp is not a function
    at onClick (ArrayControl.tsx:105)

The declaration of the two methods in the DispatchPropsOfArrayControl interface seems to be absolute identically. But it does not work.

Here is the current complete code

import range from 'lodash/range';
import React, { useMemo } from 'react';
import { ArrayControlProps, composePaths, createDefaultValue, findUISchema, Helpers, ControlElement } from '@jsonforms/core';
import { JsonFormsDispatch } from '@jsonforms/react';
import { VanillaRendererProps } from '../../index';

const { convertToValidClassName } = Helpers;

export const ArrayControl = ({
  classNames,
  data,
  label,
  path,
  schema,
  errors,
  addItem,
  removeItems,
  moveUp,
  moveDown,
  uischema,
  uischemas,
  getStyleAsClassName,
  renderers,
  rootSchema
}: ArrayControlProps & VanillaRendererProps) => {
	
  const controlElement = uischema as ControlElement;
  const childUiSchema = useMemo(
    () => findUISchema(uischemas, schema, uischema.scope, path, undefined, uischema, rootSchema),
    [uischemas, schema, uischema.scope, path, uischema, rootSchema]
  );
  const isValid = errors.length === 0;
  const validationClass = getStyleAsClassName('array.control.validation');
  const divClassNames = [validationClass]
    .concat(isValid ? '' : getStyleAsClassName('array.control.validation.error'))
    .join(' ');  
  const buttonClass = getStyleAsClassName('array.control.button');
  const labelClass = getStyleAsClassName('array.control.label');
  const controlClass = [
      getStyleAsClassName('array.control'),
      convertToValidClassName(controlElement.scope)
    ].join(' ');
  

  return (
    <div className={controlClass}>
      <header>
        <label className={labelClass}>{label}</label>
        <button
            className={buttonClass}
            onClick={addItem(path, createDefaultValue(schema))}
        >Add to {label}
	    </button>
      </header>
      <div className={divClassNames}>
        {errors}
      </div>      
      <div className={classNames.children}>
        {data ? (
          range(0, data.length).map(index => {
            const childPath = composePaths(path, `${index}`);
            return (
              <div><JsonFormsDispatch
                schema={schema}
                uischema={childUiSchema || uischema}
                path={childPath}
                key={childPath}
                renderers={renderers}
              /><button
              	aria-label={`Delete`}
                onClick={() => {
                	if (window.confirm('Are you sure you wish to delete this item?')) {
                       removeItems(path,[index])();
                    }
                }}>Delete</button>
                
                <button
              	aria-label={`Up`}
                onClick={() => {
                	if (window.confirm('Are you sure you wish to move this item up?')) {
                       moveUp(path,0)();
					}                    
                }}>Up</button>
                 <button
              	aria-label={`Down`}
                onClick={() => {
                	if (window.confirm('Are you sure you wish to move this item down?')) {
                       moveDown(path,0)();
					}                    
                }}>Down</button>
                
                
                </div>
            );
          })
        ) : (
            <p>No data</p>
        )}
      </div>
    </div>
  );
};

@rsoika
Copy link
Contributor Author

rsoika commented Aug 22, 2022

I invested more time but I can't find out why the runtime complains about that moveUp/moveDown is not a function

The only thing I found was that the util/array.ts declares methods with the same name in line 35

@sdirix
Copy link
Member

sdirix commented Aug 24, 2022

I invested more time but I can't find out why the runtime complains about that moveUp/moveDown is not a function

I think the problem is not in the posted code but in ArrayControlRenderer as this is actually the renderer which is bound against the JSON Forms React bindings and then hands over all its props to the ArrayControl. It's not destructuring the moveUp and moveDown helpers, nor handing them over, therefore they are undefined for you. Therefore just adding them there and handing them over should fix the problem for you.

Actually I don't think this separation of ArrayControlRenderer and ArrayControl makes much sense. Feel free to consolidate them into one component/file if you like to.

Note that:

  • I don't recommend warning before moving. As this is easily revertible I would just execute the move
  • You should use a key for each div in the range-map. Probably just the array index.

....As an old Java developer, I fear I will never have the chance to recognize the 'beauty' of this kind of coding... ;-)

This is not really a Javascript pattern but just bad naming on our part, lol. They should really be named removeItemsGen or removeItemsGenerator. I guess in the Java world they would be factories, e.g. removeItemsFactory.create(path, Collections.singletonList(index).toArray()).execute() 😄

@rsoika
Copy link
Contributor Author

rsoika commented Aug 24, 2022

Yes you were right (of course) - the problem was the missing definitions in the ArrayControlRenderer. Thanks, now I can start putting all together...

  • Of course I also don't plan a warning for moving up/down - this was just some kind of debugging code ;-)
  • yes I will add a key for each div - I already realized the runtime warning

I will also try to merge both classes into one component file....

@rsoika
Copy link
Contributor Author

rsoika commented Aug 25, 2022

Now I think my new version should be fine. Please take a look at my new pull request:

#2011

@lucas-koehler
Copy link
Contributor

Closed via #2011

@lucas-koehler lucas-koehler added this to the 3.0 milestone Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants