Page MenuHomePhabricator

Understand commas as search term separators
Closed, ResolvedPublic3 Estimated Story Points

Description

Motivation
Tests have shown that the currently used OOjs UI tag widget does not work well in the search use case. Although they have a very pleasing design, and make it clear where a search term starts and ends, users have difficulties understanding how to create them.
Intuitively, most users used a comma to separate search terms when searching. Therefore for now, we will implement the visually less pleasing, but better understandable alternative as described below, at the same time looking for a solution that combines the best aspects of both worlds.

Task
Make all appropriate parts of our search form react to commas as input separators.

Acceptance Criteria

  • The search boxes have "usage hints" as shown in the mock.
  • Search fields where multiple inputs are allowed, display a placeholder text that describes the field's usage regarding separating search terms (see mock). The message is the same for all appropriate search fields
  • Pasted content is automatically separated by commas
  • When the user clicks "Search", all commas are considered input separators
  • When the user expands the search bar again, all items are again separated by comma

Event Timeline

Lea_WMDE created this task.Aug 18 2017, 9:19 AM
Restricted Application added a project: archived--TCB-Team. · View Herald TranscriptAug 18 2017, 9:19 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Lea_WMDE set the point value for this task to 3.Aug 18 2017, 9:19 AM

Because our fields are very close to each other we would need to move them apart on focus to be able to display the help text. I would suggest for the MVP to try if we can sufficiently explain the behavior with the usage hints in the fields and iterate on that post MVP.

Additionally I would make the comma act exactly like pressing enter, instead of converting the comma secretly in the background, i.e. when the comma is pressed, a tag is created.

The tags should then stay tags when the user expands the search bar again.

If something is not clear, please ask.

Hey @gabriel-wmde , @Tobi_WMDE_SW ,

what is the current progress on this?

We're planning to do more user tests on the 9.10. and it would be great to have this functionality already deployed by then.

kai.nissen updated the task description. (Show Details)Oct 11 2017, 11:46 AM
kai.nissen renamed this task from Understand commas as search term seperators to Understand commas as search term separators.Oct 11 2017, 1:13 PM
This comment was removed by JeroenDeDauw.
JeroenDeDauw moved this task from Todo to Doing on the WMDE-Fundraising-Sprint-13 board.
This comment was removed by JeroenDeDauw.
JeroenDeDauw added a comment.EditedOct 12 2017, 5:34 AM

The search boxes have "usage hints" as shown in the mock.
Search fields where multiple inputs are allowed, display a placeholder text that describes the field's usage regarding separating search terms (see mock). The message is the same for all appropriate search fields

Are those two ACs the same? Or is the first one about the info thinghy that you can click on the right? If the latter, then that seems misplaced in this task.

Pasted content is automatically separated by commas

Between each word? I suppose that the desired behavior is to immediately show a box for each word and never show a comma. Correct?

JeroenDeDauw added a comment.EditedOct 12 2017, 5:37 AM

I'm running into some weird issue where only part of the text is showing after you open the search form thing.

When clicking inside of the field the full text appears. I figured this might just be a FailFox thing, but it also happens on NSAmium.

Anyone ever seen something like this?

JeroenDeDauw added a comment.EditedOct 12 2017, 7:43 AM

TODO: loading of initial values and using stuff after form submission. Might already work, did not check yet. Edit: appears to be working. Probably good if someone better familiar with the extension double checks that the behavior did not change.

@WMDE-Fisch: Do you have a guess what causes above issue with the not-totally-visible-in-field-text?

That seems to be related to the way OO UI builds the input boxes. The "real" input boxes under the styled field seems to be to small for the placholder. This should fix it:

.mw-advancedSearch-container input {
	width:100%;
}

But might be something to report to the OOui guys.

Are we following the Gerrit/Commit message guidelines to, amongst other benefits, link change sets here automatically?

JeroenDeDauw removed JeroenDeDauw as the assignee of this task.Oct 17 2017, 9:40 AM

Are we following the Gerrit/Commit message guidelines to, amongst other benefits, link change sets here automatically?

I am not it seems. So I'd just need to do bug: T123 instead of posting a link to the phab issue right? Probably will do both in that case. Just doing bug: T123 is derp, as people looking at the history that don't know phabricator have no idea what this is about.

Hey,

so I've read the thread on T175826 and one thing makes me think I need to rethink the current functionality of the fields:

Placeholders are total crap when it comes to RTL.

Well, that's totally what we've currently got in our mocks.

In don't know exactly what the status of this ticket is, but I will give this a makeover this week based on the new information I got from Moriel and Amir.

I +2'ed all the changes as I do not have general objections, and we should probably start with a clean slate...

Two things

  • when submitting and re-rendering the form with data inside our tagMultiselectWidget fields, even though there is no placeholder shown, that width: 100% is applied - which causes the (empty) input to be on a line by itself
  • still no tests

when submitting and re-rendering the form with data inside our tagMultiselectWidget fields, even though there is no placeholder shown, that width: 100% is applied - which causes the (empty) input to be on a line by itself

This is unfortunate... If you click on the field, its input field is being resized properly. When populating the field using populateFromStore() the method updateInputSize() is also called, but does nothing, because the element is not visible.
(see https://phabricator.wikimedia.org/diffusion/GOJU/browse/master/src/widgets/TagMultiselectWidget.js;c822a3e34758e0818abce4d59b6c87f90aa5f8bb$700-704).

Pablo-WMDE moved this task from Review to Doing on the WMDE-Fundraising-Sprint-13 board.
  • addressed input placeholder behavior
  • added test skeleton and the first bunch of test (@JeroenDeDauw)
Pablo-WMDE removed Pablo-WMDE as the assignee of this task.Oct 24 2017, 10:37 AM
Pablo-WMDE moved this task from Doing to Todo on the WMDE-Fundraising-Sprint-13 board.

So what still needs to be done? Adding more tests?

Some more tests in https://gerrit.wikimedia.org/r/387159

To be honest I'm finding it quite difficult to tell what to test. The control flow and encapsulation of this code... leave something to be desired, and it is often not clear what is an "internal" detail.

Pablo-WMDE added a comment.EditedNov 1 2017, 5:22 PM

Bread and butter of the the UI elements is that their state gets persisted in the store - I added tests for this.

This identified two issues:

  • ArbitraryWordInput still relies on init to to persist its state into store (via generic change) instead of doing so for itself
  • the input event used makes this really hard to test as it is only triggered when used via a real GUI, otherwise we have to manually call buildTagsFromInput which feels very redundant. Using other events (change comes to mind) interferes with other lifecycle events, however...

Maybe @gabriel-wmde should take a look, and if he doesn't have a remedy we live with this until we have a better idea.

Lea_WMDE added a comment.EditedNov 2 2017, 9:36 AM

I just realized yesterday that "This word" and "Not this word" are currently not treated in the way described here. Since there have been multiple people discussing, I don't know how to best proceed in this, but if I were to be the person to acknowledge completed ACs, I would only accept the story if they are, too, fields that react on comma with creating pills and have a placeholder text, since they allow multiple inputs. I know that there are concerns why "This word" and "this text" are seperate fields, and this issue should also be addressed (but in another ticket). For now, though, I much rather want to have a consistent UI instead of two different ways to input things.
However, if you feel like "But with Tobi we all wholeheartedly agreed on the current solution" I will have to create a new ticket I guess. Looking at the current ACs though, I don't see why these two fields should be treated differently.
@gabriel-wmde @Pablo-WMDE @JeroenDeDauw @kai.nissen

I always objected to making field 1 and 3 to have the same UI as field 2 and 4 because, in my mind, everything would have then been in quotes, which would have made at least field 1 and 2 the same functionality. After discussing this with Lea yesterday, I am more open to the idea. For unifying the 4 fields as "pill fields" we need to do the following:

  • DO NOT quote the words from the 1st, as that would destroy stemming.
  • Have some way to reject spaces in the 1st field. @Charlie_WMDE I can imagine error messages or treating spaces like commas (which might be confusing). Or maybe you have another idea?

It still seems weird to me that we then have same-looking fields that function differently (some reject spaces, some allow them). Isn't it better to indicate different space handling by having different UI elements?

IMHO this issue is just about changing the behavior of pill fields, so please do create another issue.

Lea_WMDE closed this task as Resolved.Nov 24 2017, 12:10 PM
Lea_WMDE claimed this task.