Problem/Motivation
The Node Search Settings section of the Search Settings page uses the term "weight" on its ranking multipliers. This is confusing, because it is not at all the same type of "weight" as in other places in the Drupal UI. It's actually a multiplier.
Proposed resolution
Fix the UI so that it doesn't call the search ranking multipliers "weights".
Remaining tasks
Patch is ready, needs final usability review and commit.
User interface changes
Search Settings page UI has different text on it, not using the word "weight".
API changes
None. This is only a UI change.
Original report by tgeller
I'm looking at the Weight pop-up menus in the Content Ranking section of the Search settings page (/admin/settings/search), and see three ways that they act counter to similar pop-up menus elsewhere in Drupal. (To compare, consider the Block administration page, /admin/structure/block.)
1. Not draggable.
[To see the next two points, turn off JavaScript in your browser and reload both pages.]
2. Values are from 0-10, instead of centering around 0 (e.g. -50 to 50)
3. "More important" is shown by a more-positive number: i.e. a value of 8 gives causes things to float to the top more than a value of 5. Elsewhere, it's the other way around.
Not being a programmer, I don't know how hard this would be to fix, or how you'd go about it. I'm hoping you could reuse some of the code from those other modules, but what do I know? ;)
Good luck!
Comment | File | Size | Author |
---|---|---|---|
#38 | 535616-d7port-38.patch | 1.37 KB | rootwork |
#32 | search-535616-32.patch | 1.26 KB | ianthomas_uk |
#31 | 535616-d7port.patch | 1.36 KB | jhodgdon |
#3 | rankings-before.png | 8.86 KB | jhodgdon |
#3 | rankings-after.png | 13.14 KB | jhodgdon |
Comments
Comment #1
tgeller CreditAttribution: tgeller commentedAnother point for comparison: When you add a menu link (e.g. /admin/structure/menu-customize/main-menu/add), it's a Weight pop-up menu that goes from -50 to 50.
Comment #2
jhodgdonOh gracious. I think this is a UI bug, not a feature request. It should be a fairly easy patch.
Comment #3
jhodgdonI looked into this a bit.
The problem is actually that the value shown on the screen is not a "weight" as used elsewhere in the Drupal UI (i.e. an ordering of items in a list). It's actually a multiplying factor for calculating rankings of search results. So having values from 0 to 10 is actually the right thing, and a drag-drop interface is not the right thing.
There is a bug though: the UI should not be calling this "weight", probably, but "multiplier" or something...
Also, I noticed that this part of the settings form in D7 had some help information that was not being rendered, due to a typo (it was #value and not #markup in the form definition).
Here's a fix, and before/after screen shots.
Comment #4
jhodgdonTagging.
Comment #5
jhodgdonWrong patch, sorry!!! I was working on something else and forgot to revert it. Try this one.
Comment #6
jhodgdonFixing title to reflect actual issue
Comment #7
jhodgdon#5: 535616.patch queued for re-testing.
Comment #8
jhodgdonSee also
#834792: semantics in interface translation
Comment #9
jhodgdon#5: 535616.patch queued for re-testing.
Comment #10
jhodgdon#5: 535616.patch queued for re-testing.
Comment #11
jhodgdon#5: 535616.patch queued for re-testing.
Comment #12
jhodgdontagging
Comment #13
jhodgdonThere is zero chance this is making it into D7 at this point, since no one cared to review the patch for more than 6 months. Bumping to D8 for consideration, although hopefully we'll start over with a new search framework for D8 and this will be irrelevant.
Comment #14
oriol_e9gProper D8 patch.
Comment #15
Bojhan CreditAttribution: Bojhan commentedWhy are we adding all this help text, I do not see how its much more clear?
Comment #16
kika CreditAttribution: kika commented+1 for this change, was confused about this table myself and was almost suggesting tabledrag component. Please do it.
PS. Influence would be an awesome case for #1174646: Add new HTML5 FAPI element: range
Comment #17
kscheirer#14: string-improvements-535616-14.patch queued for re-testing.
Comment #18
kscheirerRetesting against latest HEAD since it has been over a year.
Comment #19
rootworkI hope this can get into 8. I just ran into this confusion installing a new D7 site.
I suspect the reason there isn't more attention paid to this is because "power" users replace Drupal core search with other things like Solr. But the built-in search settings page shouldn't say "weight" when it doesn't mean the same thing it means everywhere else.
Comment #20
jhodgdonThe patch will need to be rerolled because this code is now I think in the NodeSearch plugin. But I think it's a good patch... can someone reroll it and we'll get it into 8 hopefully?
Comment #21
pwolanin CreditAttribution: pwolanin commentedre-roll for the plugin change.
Comment #22
jhodgdon21: 535616-21.patch queued for re-testing.
Comment #23
jhodgdonpwolanin and I both think this is a sensible fix, and it is just UI text, so I am tentatively marking it RTBC but also tagging Usability in case any of the usability folks want to weigh in. I also just requested a re-test, just in case.
And I'm redoing the issue summary.
Comment #24
catchI got a bit confused scanning the text by 'higher influence numbers', makes me think of 7 or 13 - maybe one more look over for that?
Comment #25
ianthomas_ukThis will conflict with #2042807: Convert search plugins to use a ConfigEntity and a PluginBag and could maybe just be changed in the patch for that issue.
Comment #26
jhodgdonRE #25 - Actually, no, we don't want to combine issues together and fix a lot of unrelated stuff on that issue. We'll just need to live with a few rerolls, depending on which issue lands first.
RE #24... good point.
So we've changed the UI so that the table headers are "Factor" and "Influence" instead of "Factor and "Weight" (see screen shots in #3). How about if the text above the table says:
Influence is a numeric multiplier used in ordering search results. A higher number means the corresponding factor has more influence on search results; zero means the factor is ignored. Changing these numbers does not require the search index to be rebuilt. Changes take effect immediately.
Here's a new patch, which will hopefully apply since I just (gasp!) edited the previous patch directly. Interdiff seems kind of pointless on a two-line patch. :)
Comment #27
pwolanin CreditAttribution: pwolanin commentedLooks like a better explanation - let's get this little fix in.
Comment #28
jhodgdonI was going to say that tim.plunkett would be sad if this gets committed, but I do not believe it will conflict with #2042807: Convert search plugins to use a ConfigEntity and a PluginBag so it should not be a problem. It touches a few lines of the same file (NodeSearch), but they do not overlap so I think it will be OK.
Comment #29
webchickConfirmed the two do not conflict.
Committed and pushed to 8.x. Thanks!
Do we want to backport this to D7 as well, since it's just a string change?
Comment #30
jhodgdonSure, and it should be an easy patch to port.
Comment #31
jhodgdonHere is a straight port of this UI-text-only patch to Drupal 7.x.
Comment #32
ianthomas_ukIt looks like the fix to show the help text got lost in a reroll, here's a patch to add it back again. I've also added a
<p>
tag for consistency and to give the text a bit of space.Comment #33
jhodgdonDoh! Thanks, this is good.
Comment #34
jhodgdon(in other words, I applied the patch and tested manually by going to an edit page for the Node Search page config and the text now shows up)
Comment #35
jhodgdon32: search-535616-32.patch queued for re-testing.
Comment #36
webchickCommitted that follow-up, back to 7.x.
Comment #37
jhodgdonThe 7.x patch in #31 needs a similar update to #32. The help text is not shown there either.
Comment #38
rootworkHow's this?
Comment #39
rootworkI know I shouldn't review my own patch, but I figured I'd post screenshots at least.
Without patch:
With patch:
Comment #40
jhodgdonScreen shots are always welcome!
So, this patch is a straightforward port of the D8 patch and it works, so I think it is ready to be RTBC.
Comment #42
TanvirAhmad CreditAttribution: TanvirAhmad commented32: search-535616-32.patch queued for re-testing.
Comment #44
jhodgdonThere was a test failure here in ComentPreviewTest. The log says:
Unrelated to this patch. I don't think we need to click "Retest" here. The bot will test all RTBC patches every 24 hours anyway.
Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commentedDrupal 7.27 was a security release, so this is now scheduled to go out in Drupal 7.28 instead.