Skip to content
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

[form-builder] Add overridable part for array input functionality #795

Merged
merged 5 commits into from May 8, 2018

Conversation

rexxars
Copy link
Member

@rexxars rexxars commented May 7, 2018

Sorry for the vague description.

Basically, we have seen people wanting to customize the process of adding new items to an array, but once you go down that path you quickly come to a point where you might want to perform other operations, such as adding a button for sorting the array items alphabetically or similar.

This PR moves the "Add"-button logic into a separate part and makes both the primitive and object version of array inputs use it. I had to do a few generalization changes, but it does lead to more code reuse.

There is an example added in the tests studio where it allows you to add items by searching the list of possible types.

Have not had time to give the API as much thought as I would have wanted, open for suggestions, feel free to make changes directly in the PR.

Copy link
Contributor

@kristofferjs kristofferjs left a comment

Choose a reason for hiding this comment

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

Tested locally. Seems to work fine.

Copy link
Member

@bjoerge bjoerge left a comment

Choose a reason for hiding this comment

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

A few comments/nits, otherwise LGTM

@@ -3,6 +3,7 @@ import type {ArrayType, ItemValue} from './typedefs'
import React from 'react'
import DropDownButton from 'part:@sanity/components/buttons/dropdown'
import Button from 'part:@sanity/components/buttons/default'
Copy link
Member

Choose a reason for hiding this comment

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

☝️Neither DropdownButton or Button is in use in this file anymore and these imports can be removed.

@@ -3,6 +3,7 @@ import React from 'react'
import {get} from 'lodash'
import {Item as DefaultItem, List as DefaultList} from 'part:@sanity/components/lists/default'
import {Item as SortableItem, List as SortableList} from 'part:@sanity/components/lists/sortable'
import ArrayFunctions from 'part:@sanity/form-builder/input/array/functions'
import Fieldset from 'part:@sanity/components/fieldsets/default'
import Button from 'part:@sanity/components/buttons/default'
Copy link
Member

Choose a reason for hiding this comment

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

The Button and DropdownButton imports can be removed from this file too

readOnly={readOnly}
appendItem={this.append}
prependItem={this.prepend}
focusItem={this.handleFocusItem}
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this prop is not named onFocusItem?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a bit torn on this - onFocusItem sounds like an event handler to me. "When an item is focused, call this function", but this is the opposite - it's an action: "Focus the item with the given path for me". Same goes for the other items, appendItem, focusItem and createValue are also actions, not event handlers.

Copy link
Member

@bjoerge bjoerge May 8, 2018

Choose a reason for hiding this comment

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

I see your point. Personally, I like to think about it from the component's perspective, and let it emit it blindly without having to consider context/what should happens next. When looking at it from the component's perspective it makes more sense to think of it as an event , e.g. "user requests the item to be focused/appended/whatever". So I think it should be up to up to the owner/consumer component to decide how to translate these events into the appropriate actions (or emit other events).

Copy link
Member Author

Choose a reason for hiding this comment

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

Will change for the sake of consistency.

children: ?Node,
value: Array<ItemValue>,
readOnly: ?boolean,
appendItem: (itemValue: ItemValue) => void,
Copy link
Member

Choose a reason for hiding this comment

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

Unless there's a good reason that I'm missing, I would prefer prefixing the function props here with on, e.g. onAppendItem. That is more in line with the conventions used elsewhere.
So

  • appendItem => onAppendItem
  • focusItem => onFocusItem,
  • createValue => onCreateValue

onChange: (event: PatchEvent) => void
}

export default class ArrayFunctions extends React.Component<Props, State> {
Copy link
Member

Choose a reason for hiding this comment

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

This component has no state, so it should be just extends React.Component<Props>

@rexxars
Copy link
Member Author

rexxars commented May 8, 2018

Updated to address review feedback, please take another look :)

@rexxars rexxars merged commit 7b4bcb1 into next May 8, 2018
@rexxars rexxars deleted the add-array-item-part branch May 8, 2018 09:13
bjoerge pushed a commit that referenced this pull request May 14, 2018
* [form-builder] Add overridable part for array functionality

* [test-studio] Add example of overriding array functions

* [form-builder] Reorganize imports according to lint config

* [form-builder] Remove state declaration from ArrayFunctions as it is stateless

* [form-builder] Add `on`-prefix to append/prepend/createValue/focusItem
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.

None yet

3 participants