In #2574969: Add a Views-like UI for adding fields we overhauled the Fields UI, roughly basing it on Views. However, a lot of it is still makeshift, and to really reap the UX benefits of the new layout, we'll have some improvements still to do:

  • Make the "Add fields" page a popup dialog opened on the Fields tab, add fields via AJAX.
  • The "Remove" links should also use AJAX, although that's probably not as critical. (The current ones will still discard all changes to field types/boosts (and later names and IDs, too) when removing a field, so that would be annoying.)
  • The "Add fields" page (or popup) works well, currently, but looks ugly (or "functional", if you want to be polite). Some love from a designer and/or UX expert would be great there. And, in any case, browsing through the different datasources and nested properties should of course also use AJAX.

Of course, all of this should also degrade well (i.e., work more or less like now) if Javascript is disabled.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

drunken monkey created an issue. See original summary.

janusman’s picture

I tried this out in its current state, and have some feedback.

What's good:

* I like the idea of separating the adding of fields as a separate screen/set of screens, to reduce cognitive load.

Problems:

* Like a restaurant visitor, you sometimes just want to scan the menu; you don't really know what you want. So, the current state of things hides the "restaurant menu" behind a lot more clicks.
* New user would have no idea from the start what to do when first shown the "add fields" screen: "What's 'General'? What's 'Content'? I guess I'll click on one and find out".
* I don't see the '+' being used elsewhere in Drupal core, so I was not sure if "+" would add something or expand.
* No indication of what I've already added.
* Some items (like taxonomy terms) seem to have both a "+" and an "Add" button. Clicking on "+" does nothing.

My opinions:

* IIUC, we re trying to mimic something like core's "Add block" page, in that we can add items, you can add more than one of each 'type', and each item added has its own configuration/settings. If so, then I wonder if we should stick to more completely mimicking that workflow that is recognizable/known to users, vs. creating a new one. We would then not really have expandable items, since we would mimic the selector used in views, in blocks, etc. where we have things as categories and we can filter that way.

If we don't want to go this way... then some things that might improve the new workflow:

* make the expandable items "(+)" and "(-)" links to look like (or be) fieldsets, because those are in use elsewhere and readily recognizable by all as 'expandable'
* expand the first level by default

drunken monkey’s picture

Thanks a lot for your review! Some very good points in there, this will surely help a lot in making the page more usable.

* IIUC, we re trying to mimic something like core's "Add block" page, in that we can add items, you can add more than one of each 'type', and each item added has its own configuration/settings. If so, then I wonder if we should stick to more completely mimicking that workflow that is recognizable/known to users, vs. creating a new one. We would then not really have expandable items, since we would mimic the selector used in views, in blocks, etc. where we have things as categories and we can filter that way.

The problem I had to solve here is that we don't have a fixed list of possible properties, but people can, in theory, go arbitrarily deep for the properties they want to add. You could index a node author's taxonomy terms' parents' related nodes' comments' authors' roles, if you want to. So putting the available properties on one page just isn't possible – at least without some additional UI for choosing which properties should be expanded there, which we previously had. But I think the drill-down pattern makes more sense there, because otherwise you have to go between two different forms for getting to the properties you want. (Also, with too many properties enabled there, the list could still become too long on larger sites, maybe even crashing them.)

For the same reason, I couldn't use collapsed fieldsets – they also require the collapsed data to already be there. But I guess we can try to style the links to look like collapsed fieldsets – that should make it much clearer what this does.

Always shwoing the first level for each datasource makes sense in any case, of course, thanks!

* No indication of what I've already added.

That's an interesting one, didn't think of that.
As explained in the issue summary, though, the plan is to have this form as a modal popup on the "Fields" tab, so maybe that will already help somewhat. But we can then surely review whether an additional indicator for fields that were already added still makes sense. (Though Views, e.g., also doesn't have that.)

* Some items (like taxonomy terms) seem to have both a "+" and an "Add" button. Clicking on "+" does nothing.

That seems like a bug, then, and one I cannot reproduce. A taxonomy term reference field should expand to a single "Taxonomy term" property, which will in turn expand to the list of its properties. (That additional indirection is of course rather pointless, but dictateted by Drupal's inner structure. I plan to get rid of that eventually, too.

In any case, thanks again for your feedback!

borisson_’s picture

You could index a node author's taxonomy terms' parents' related nodes' comments' authors' roles, if you want to.

Hah, you probably shouldn't go that far, but yeah, I figured that this would be the reason for not showing everything already.

For the same reason, I couldn't use collapsed fieldsets – they also require the collapsed data to already be there. But I guess we can try to style the links to look like collapsed fieldsets – that should make it much clearer what this does.

I don't think it's a good idea to fake fieldsets in this case.
Is there any other module that requires drilling down that we can talk to - to create a similar experience?

Always showing the first level for each datasource makes sense in any case, of course, thanks!

Totally agree! Let's do that.

* No indication of what I've already added.

That's an interesting one, didn't think of that.
As explained in the issue summary, though, the plan is to have this form as a modal popup on the "Fields" tab, so maybe that will already help somewhat. But we can then surely review whether an additional indicator for fields that were already added still makes sense. (Though Views, e.g., also doesn't have that.)

What will we do with fields that are added multiple times, this is possible and should they have 2, 3, 4 indicators as well? Not sure about that.
The modal will already help A little bit to reduce the disconnect, but when we do that, we should probably have the background update as well if we want to see what fields are already added. I'm don't think that'll be easy (or a good idea) to do.

Awesome review though, many thanks @janusman!

Arla’s picture

I'm trying to update the ever-pending Search API test for Collect: #2420777: Tests for Search API integration One step is to test adding some fields provided by the Collect datasource plugin. While the new UI has many advantages to usability, it is unfortunately very hard to use from within a web test. Did you already consider this, am I missing something?

One solution, from the top of my head, could perhaps be to name the "Add" buttons after the datasource ID and property path, similar to what I can see in the URL.

Edit: Oops, seems like my suggestion is exactly how it already works ;)

Arla’s picture

Despite the addition to my last post, I still cannot find out how to use the form correctly from a web test. I thought I could use drupalPost(Ajax)Form for a given button name, but it seems the button value is all I can go by, which is to no help when they all have "Add".

drunken monkey’s picture

Yes, that's become tricky, I fear. But I already had to solve it for our own integration test: see \Drupal\search_api\Tests\IntegrationTest::addField():

  protected function addField($datasource_id, $property_path, $label = NULL) {
    $path = $this->getIndexPath('fields/add');
    $url_options = array('query' => array('datasource' => $datasource_id));
    if ($this->getUrl() === $this->buildUrl($path, $url_options)) {
      $path = NULL;
    }

    // Unfortunately it doesn't seem possible to specify the clicked button by
    // anything other than label, so we have to pass it as extra POST data.
    $combined_property_path = Utility::createCombinedId($datasource_id, $property_path);
    $post = '&' . $this->serializePostValues(array($combined_property_path => $this->t('Add')));
    $this->drupalPostForm($path, array(), NULL, $url_options, array(), NULL, $post);
    if ($label) {
      $args['%label'] = $label;
      $this->assertRaw($this->t('Field %label was added to the index.', $args));
    }
  }
mikemiles86’s picture

phenaproxima’s picture

Regarding the drilling down -- isn't this pretty much exactly what Hierarchical Select was for?

drunken monkey’s picture

Thanks for the tip!
I think depending on it is out of the question, but it might provide hints how to best implement this ourselves.

phenaproxima’s picture

@drunken monkey, Hierarchical Select should definitely not be a dependency of Search API -- it's quite a complex module and is written to cover many different use cases. But its basic functionality is relatively simple and could be re-implemented as a reusable widget (which, IMO, should go into core -- but that's a different discussion) in Search API.

borisson_’s picture

drunken monkey’s picture

I don't think we need that relationship in both directions.

mallezie’s picture

Assigned: Unassigned » mallezie
FileSize
65.67 KB
523.84 KB

Talked a bit with bojhan and _borisson about it. A suggestion was to do something token UI interface like.

This would look like the attached files.

Clicking the add field button, opens a model, with a 'hierarchical' table as in token. Clicking the link adds the field.
Gonna give this a try today.

drunken monkey’s picture

Issue tags: +DevDaysMilan

Yes, that would be awesome!
Thanks a lot!

borisson_’s picture

@mallezie: can you post the work you did for this? Even if it's not done yet, it's probably a good start for us to work with.

mallezie’s picture

FileSize
16.81 KB

Sorry, work picked up faster then expected.

Attached fully non functional patch. ;-)
Adjusten action link to link, since action link can't open modals.
Open things in modal.
Started ajaxifying things, now stuck on opening one plus, doesn't keep the previous ones open.
Gonna unassign for now, since juli is mostly holiday for me actually. Feel free to pick up, else i'll try to pick up further later.

drunken monkey’s picture

Assigned: mallezie » Unassigned
Status: Active » Needs work

Sorry, work picked up faster then expected.

It always does …
But still pretty good response time – others have been known to sit on such unfinished patches for months. ;)

Gonna unassign for now, since juli is mostly holiday for me actually. Feel free to pick up, else i'll try to pick up further later.

OK, sure. We'll see whether someone else feels like it, otherwise it would of course be great if you could continue work on it at some point.
In any case, thanks a lot for your work so far!

drunken monkey’s picture

Issue tags: +Release blocker
mathysp’s picture

Assigned: Unassigned » mathysp

I looked over the WIP patch by @mallezie, but applying this patch causes some issues. I discussed this with @mallezie and he confirms that there are some issues with it.

I'm not really familiar with the AJAX part, so today I will focus on styling the default way. The work done by @mallezie will not be used as a starting point.

Maybe afterwards I can dive deeper into the AJAX part.

mathysp’s picture

Assigned: mathysp » Unassigned

I have to let this one go. Got a bad case of the drupalcon-flu and had to go lie down. DIdn't really achieve anything notable, so no patch.

dazz’s picture

Assigned: Unassigned » dazz

I'll take a crack at it.
Idea is to add a button "Add fields" bellow every data source that opens a popup where you can select fields token'isch style for that source.
Fields will be added ones you click save in the popup.

To fix the problem of not seeing what fields are select I was thinking something in the line of a split screen popup where you can select the fields on the left and a list of selected fields on the right.
The only alternative I see is to add fields one at a time and closing the popup each time, but I believe that is just annoying if you have to add a lot of fields.

drunken monkey’s picture

To fix the problem of not seeing what fields are select I was thinking something in the line of a split screen popup where you can select the fields on the left and a list of selected fields on the right.

Doesn't Views already do something like that – listing the selected fields/filters/etc. once again at the bottom of the popup? (We'd just need to also add the previously added fields there.)

In any case, your suggestion sounds pretty good. Would be awesome if you could provide a patch for that!
(Probably we'll have to get rid of the "Add fields" action link anyways, since we might have a hard time AJAXifying that. In that case, having one link per datasource could be an improvement.)

drunken monkey’s picture

Assigned: dazz » Unassigned
FileSize
16.13 KB
13.98 KB
11.7 KB

I don't think you're working on this anymore? If so, I guess I'll have to take a shot.
Otherwise, please re-assign. I won't be able to start until next week anyways.

However, I did already manage to re-roll the patch from #17 and remove all the debugging code (at least as much as I could find). See attached patches.

drunken monkey’s picture

I finally managed to find the time to tackle this and got it to work. Took a while, but it went remarkably smoothly, all in all. I ran into a few problems, some of which I couldn't resolve, but the main features all worked pretty much immediately.

The unresolved issues, input appreciated:

  1. Can we make the "X" in the top-right corner of the modal refresh the page, too?
  2. The AJAX detection code in IndexAddFieldsForm::addField() looks a bit hacky. Does anyone know a more elegant (ideally: the proper) way to detect whether we're inside an AJAX request? (Same for all other such conditions in the various form/controller classes.)
  3. Originally, I had "nojs" and "ajax" paths for removing fields, too, but that didn't work properly with the CSRF tokens. Does anyone know a solution to that?
  4. The expand/collapse links in the "Add fields" form has a similar problem: when I tried to switch to using a callback for only returning/replacing the affected part of the form (just the HTML for that property and its children), I had to switch the links to (appropriately styled) buttons, but then I couldn't get that to work in a no-JS browser. Probably would have worked in the end, but it seemed like too much trouble to fix something that was basically working anyways.

For the latter two, I'm also attaching the failed attempts, in case someone wants to take a look. But I'm pretty satisfied with this result even if we don't manage to resolve those last problems – it still works pretty well, and is a vast improvement over what we currently have.

In any case, please test and review, so we can finally check off this release blocker, too!

(Currently, the tests should already verify that this continues to work fine in a no-JS browser. Adding tests that also test the Javascript would of course be great, but that's probably a lot of effort, so I think we can skip that for now, too. Should be a follow-up, though.)

Status: Needs review » Needs work

The last submitted patch, 25: 2641388-25--add_fields_ajax.patch, failed testing.

drunken monkey’s picture

I think item 3 above is actually a Core bug: #2857412: AJAX links don't work with CSRF tokens.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
4.18 KB
30.68 KB

This should fix the tests. Small oversight. (Although, if we end up with a different way of detecting AJAX vs. normal request, I guess the duplicated route doesn't really serve a purpose anyways.)

borisson_’s picture

Status: Needs review » Needs work

Overall this code looks great, I haven't manually tested but I have some questions / nits.

I also think we should change all the array calls that we touch here to short array syntax (now that we use those)

  1. +++ b/src/Form/FieldConfigurationForm.php
    @@ -87,6 +94,21 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      $form['title']['#markup'] = new FormattableMarkup('<h2>@title</h2>', ['@title' => $form['#title']]);
    +      Html::setIsAjax(TRUE);
    

    Why are we using FormattableMarkup here instead of $this->t that we use just a few lines above it? I also think that we shouldn't use the <h2> in the translated string, we should probably use #prefix and #suffix to add the header information.

  2. +++ b/src/Form/IndexAddFieldsForm.php
    @@ -188,52 +203,25 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    if ($properties) {
    

    Shouldn't this be !empty() instead?

  3. +++ b/src/Form/IndexFieldsForm.php
    @@ -284,12 +314,26 @@ protected function buildFieldsTable(array $fields) {
    +        // Could not retrieve data definition: ignore.
    

    Let's add why we can ignore this in the comment here? Does this catch the $field->getDataDefinition call?

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
31.04 KB

Thanks a lot for your review!

I also think we should change all the array calls that we touch here to short array syntax (now that we use those)

I thought about that, but decided against it. We'll just roll a single patch for all remaining places in the code during RC.

Why are we using FormattableMarkup here instead of $this->t that we use just a few lines above it? I also think that we shouldn't use the <h2> in the translated string, we should probably use #prefix and #suffix to add the header information.

It's formattable, not translatable. I hope that resolves all of these remarks.

Shouldn't this be !empty() instead?

I don't like using !empty() in cases where I don't need an isset(). Since that variable is always set, I prefer the plain if ($properties).

Let's add why we can ignore this in the comment here? Does this catch the $field->getDataDefinition call?

Yes, exactly. And since that shouldn't throw an exception for processor-added properties (unless the processor is removed), we can just ignore it. Added this to the comment.

The patch also adds an "empty" message for the datasource fields tables. (The issue title says to "generally improve the new Fields UI", after all.) I also wanted to add alphabetic sorting for fields (currently, the order is the one they were added in, which I don't think is very useful), but that would lead to messy diffs when re-saving the fields form the first time, so I guess we'd better wait for #2860167: Add "orderby" to our config schema definitions with that.

Status: Needs review » Needs work

The last submitted patch, 30: 2641388-30--add_fields_ajax.patch, failed testing.

borisson_’s picture

Ok, sounds good! Patch needs a reroll though.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
31.09 KB

You're right, thanks!

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in, we can always make it better further down the line.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Yeah, you're right, thanks! Speaking of which: opened #2865997: Fix behavior of the "Save and add fields" button as a follow-up.
Anyways, committed.
Thanks again to everyone involved!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.