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

Added new setting field to configure multiple values #12807

Merged
merged 17 commits into from May 3, 2018
Merged

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented May 2, 2018

We had this developed a couple of times in various plugins and lots of duplicated code. As I now needed it as part of the settings API, I finally made it available through the API. I named it "multi pair" as in you can receive multiple values each consisting of a pair / two values. Couldn't think of a different name. With this you can basically receive a value like [{mykey1: myvalue1, mykey2: myvalue2}, {...}, ...]

You can combine various fields like text + text:
image

select + text:
image

and it is even possible to configure only one field like this:
image

Later we might change it to accept also 3 or 4 or 5 ... fields in a generic way although the main problem is positioning them nicely next to each other etc.

It is a basic, quite flexible solution that we can extend later with more features if needed.

I had to make this work with the settings/fieldConfig API so kind of misused uiControlAttributes:

            $this->makeSetting('fooBar', array(), FieldConfig::TYPE_ARRAY, function (FieldConfig $field) {
                $field->title = 'Foo Bar';
                $field->description = 'Optionally set one or multiple custom dimensions in scope visit.';
                $field->uiControl = FieldConfig::UI_CONTROL_MULTI_PAIR;
                $field1 = new FieldConfig\MultiPair('Index', 'index', FieldConfig::UI_CONTROL_TEXT);
                $field2 = new FieldConfig\MultiPair('Value', 'value', FieldConfig::UI_CONTROL_TEXT);
                $field->uiControlAttributes['field1'] = $field1->toArray();
                $field->uiControlAttributes['field2'] = $field2->toArray();
            }),

@tsteur tsteur added the Needs Review PRs that need a code review label May 2, 2018
@tsteur tsteur added this to the 3.5.0 milestone May 2, 2018
@diosmosis
Copy link
Member

Re name: I think the 2 value version could be named pair since in the code you're basically creating a n array field of a pair of values. If you made the number of fields generic, you could call it a tuple.

Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

Code looks good. I would approach the architecture differently, though. Instead of a one multipair control that is a list of pairs, there would be two UI controls, one being a list control that would manage the '-'/'+' buttons and would take a child control, which could be pair or tuple (or any other control), which would list specific inputs next to each other in groups.

if ($scope.field1 && $scope.field2) {
if (!table[$scope.field1.key] && !table[$scope.field2.key]) {
hasAll = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

If table has field1 but not field2, hasAll == true? If yes, hasAll might be named incorrectly.

Copy link
Member Author

Choose a reason for hiding this comment

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

cheers, renamed it

@tsteur
Copy link
Member Author

tsteur commented May 2, 2018

@diosmosis renamed it to multituple

@tsteur tsteur closed this May 2, 2018
@tsteur tsteur reopened this May 2, 2018
@tsteur
Copy link
Member Author

tsteur commented May 2, 2018

@diosmosis I don't 100% understand what you mean re changing the structure but could change it once we add more fields and make it more generic if it is of any help 👍

@diosmosis
Copy link
Member

diosmosis commented May 2, 2018

First off, let me say the comment isn't something I think is a requirement and I don't even know if it's possible server side. The idea is basically to separate the responsibilities of multipair out. Instead of multipair, which manages a list (add / remove), and manages pairs/tuples (set/unset field1, set/unset field 2, ...), there would be a list component that would add/remove generic fields in an array, and a pair/tuple component that would manage setting fields on an object.

As a way to visualize this, imagine a tree of components like this:

- List
    - Tuple (Index text input, Value text input)
        - Text (Index)
        - Text (Value)
    (next to tuple: Delete Icon)
    - Tuple (Index, Value)
        - Text (Index)
        - Text (Value)
    (next to tuple: Delete Icon)
    ...
(after list: Add Icon)

@diosmosis
Copy link
Member

Tested it locally, noticed some odd UX behavior:

  1. In the initial state, there's no remove/add icon:

image

Entering text suddenly adds a new row:

image

Since there's no visual cue that that would happen, I found it very surprising. It made me think I had to fill out the extra row (ie, it's not obvious it's just for adding a new item, and not a required item).

  1. Not exactly sure how, but I was able to get to state where entering text into the bottom left row didn't create a new row:

image

(seems to be when one of the previous rows is empty. doubt it would happen a lot, but I think it would be an odd surprise.)

  1. Then I tried entering something into the top row and it suddenly added a new row:

image

  1. Saving w/ two blank rows and reloading gave me an angular error I can't recover from w/o running a SQL query:

image

@diosmosis diosmosis merged commit 94ba5d2 into 3.x-dev May 3, 2018
@diosmosis diosmosis deleted the multipairfield branch May 3, 2018 19:37
@Findus23
Copy link
Member

Findus23 commented May 7, 2018

I just tried it out and it is working great.

Only "error" is that the hover of the delete icon shows The string General_Remove was not loaded in javascript. Make sure it is added in the Translate.getClientSideTranslationKeys hook.

Should I load it manually?

@sgiehl
Copy link
Member

sgiehl commented May 7, 2018

tsteur added a commit that referenced this pull request May 7, 2018
sgiehl pushed a commit that referenced this pull request May 7, 2018
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* Added new setting field to configure multiple values

* remove not needed comment

* better spacing between rows

* make fields wider

* fix ui test

* rename multipair to multituple

* rename variable

* fix it was not possible to persist nested arrays

* only catch error when using old version

* catch exception

* better implementation

* require higher version

* trying to fix json_encoded columm missing in db during updater

* silence errors

* hard code the query

* fix user login cannot be null

* fail if query fails
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants