Add a `displayField` property to the `relation` widget #594

medfreeman · 2017-09-12T02:00:48Z

- Summary

closes #591

- Test plan

Actually there's no tests for widgets ? should i make some anyway ?

- Description for the changelog

Add a displayField property to the relation widget, that allows displaying a different field that the linked valueField to the user.

- A picture of a cute animal (not mandatory but encouraged)

erquhart · 2017-09-15T21:29:06Z

src/components/Widgets/RelationControl.js

+      const searchFields = [...new Set(
+        [
+          ...field.get('searchFields').toJS(),
+          field.get('valueField'),


Can you explain the decision to move to a Set and to pass in the value field?

The move to a set (casted to an array) is just a way to ensure unique values in the array, since searchFields could contain valueField.

The actual initial search (prior to my mods) is to find a search match on the stored value.
Is its purpose to preserve relations when the target field has changed a bit ?

i.e. I change my page collection item title from "Job offers" to "Job listings".
A menu collection item that has a relation set to the "Job offers" page title field has a now a broken relation.
When i go to edit the menu, the initial search is performed and let's say an unique match is made on "Job listings" (probably because it's the only page containing the word "Job"). The relation value field will be automatically set to "Job Listings".
This behavior is not really ideal, since in this case the relation entry shows an automagically fixed relation, that is not even correct until saved, and it is really misleading.
I hope i make sense on this.

I think this behavior could be changed to only perform the search in valueField, and get the exact match needed to get the displayValue.
I really think that the main use cases of relations should be implemented by linking to a unique constant field per item, or face broken relations, and that broken relations should not be automatically corrected.

This code was just a mix of both approaches, getting the displayValue on an exact match while preserving the old behavior on a partial match.
Do you think we should keep the actual behavior ?
Or that i should change this code to

const searchFields = [ field.get('valueField') ];

to only get the initial displayValue and be done with it ?

erquhart · 2017-09-15T21:29:52Z

src/components/Widgets/RelationControl.js

+    return (
+      <div>
+        <input {...otherInputProps} value={displayValue} />
+        <input type="hidden" id={id} value={value} />


Need some explanation on this too.

The single input field was replaced by two input fields.

The first is the visible one, displaying the displayValue to the user. {...otherInputProps} is needed to preserve the field accessibility.

The second stores the value, in order for it to be saved.

erquhart · 2017-09-15T21:31:16Z

@medfreeman just took a look - some of the changes made straightforward code a bit less straightforward, and the purpose wasn't clear when skimming. In those cases we try to use source comments for clarity. Mind adding those?

medfreeman · 2017-09-16T10:03:25Z

Sure, i'll add comments. I didn't see so much use of comments througout the codebase, so i restrained, but it's never a bad thing imo.

closes decaporg#591

…`getSuggestionValue` properly in `Autosuggest` component

Only query the `valueField` on initial search Stop changing the initial `value` to avoid "Are you sure you want to leave this page?" popup on non-edited value.

Add comments

erquhart · 2017-09-19T14:54:07Z

Yeah, comments are certainly lacking throughout, but I've been working to change that. The editor came with a ton of comments, for example.

erquhart · 2017-09-19T15:45:41Z

src/components/Widgets/RelationControl.js

-      const suggestion = nextProps.queryHits.get(this.controlID);
-      if (suggestion && suggestion.length === 1) {
-        const val = this.getSuggestionValue(suggestion[0]);
-        this.props.onChange(val, { [nextProps.field.get('collection')]: { [val]: suggestion[0].data } });


This seems pretty intentional, do we know what purpose it was serving?

erquhart · 2017-09-19T15:48:27Z

docs/widgets.md

@@ -71,6 +71,7 @@ Property | Accepted Values | Description
 `collection` | string | name of the collection being referenced
 `searchFields` | list | one or more names of fields in the referenced colleciton to search for the typed value
 `valueField` | string | name a field from the referenced collection whose value will be stored for the relation
+`displayField` | string | name a field from the referenced collection whose value will be displayed to the user for the relation


Prepend with:

optional, ...

Append:

..., defaults to match `valueField`

erquhart · 2017-09-19T15:49:16Z

example/config.yml

@@ -68,6 +68,7 @@ collections: # A list of collections the CMS should be able to edit
        collection: "posts"
        searchFields: ["title", "body"]
        valueField: "title"
+        displayField: "title"


displayField should default to value of valueField so that existing implementations don't break. These examples changes can then be removed.

erquhart · 2017-09-19T15:49:31Z

src/components/Widgets/RelationControl.js

 import { connect } from 'react-redux';
 import { debounce } from 'lodash';
 import { Loader } from '../../components/UI/index';
 import { query, clearSearch } from '../../actions/search';

-
-function escapeRegexCharacters(str) {


Good catch :)

erquhart · 2017-09-19T15:51:17Z

src/components/Widgets/RelationControl.js

-        this.props.onChange(val, { [nextProps.field.get('collection')]: { [val]: suggestion[0].data } });
+      // store query results
+      const matches = nextProps.queryHits.get(this.controlID);
+      // if there is only one item found


We haven't officially set a code style guide, but I do want to keep comments looking proper. Please update the comments to follow Airbnb's comment guidelines, which I've also been loosely following (exception being that I use the multi-line style for all comments, but that isn't required).

erquhart · 2017-09-19T15:55:13Z

src/components/Widgets/RelationControl.js

+    return (
+      <div>
+        <input {...otherInputProps} value={displayValue} />
+        <input type="hidden" id={id} value={value} />


Still not understanding what this guy is doing. Can you clarify? Makes it seem like the forID prop value is getting real responsibility here, and I'm concerned it won't hold up, e.g. when nested.

erquhart · 2018-02-28T01:40:03Z

Closing due to lack of activity.

medfreeman changed the title ~~Relation widget display field~~ Add a displayField property to the relation widget Sep 12, 2017

medfreeman force-pushed the relation-widget-display-field branch from 4c6f2c1 to 919e576 Compare September 14, 2017 18:18

erquhart reviewed Sep 15, 2017

View reviewed changes

medfreeman added 7 commits September 16, 2017 13:43

chore(widget): fix eslint errors

23a21a0

docs(widgets): add displayField entry to Relation Widget section

fba569a

feat(widget): add displayField support

3bc9be3

closes decaporg#591

example(config): add displayField to relation widgets

d12d439

fix(widget): decouple getSuggestionDisplayValue to restore getting …

7292844

…`getSuggestionValue` properly in `Autosuggest` component

fix(widget): stop auto-correcting relations on partial matches

8e57884

Only query the `valueField` on initial search Stop changing the initial `value` to avoid "Are you sure you want to leave this page?" popup on non-edited value.

chore(widget): rename initial suggestion to matches for clarity

fde4f21

Add comments

medfreeman force-pushed the relation-widget-display-field branch from 8db4fbc to fde4f21 Compare September 16, 2017 11:43

erquhart suggested changes Sep 19, 2017

View reviewed changes

erquhart added area: extensions/widgets abandoned labels Feb 28, 2018

erquhart closed this Feb 28, 2018

This was referenced Mar 5, 2018

Widget docs a bit thin #619

Closed

WIP: Relations/Tags/TBD Widget Feature #1100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `displayField` property to the `relation` widget #594

Add a `displayField` property to the `relation` widget #594

medfreeman commented Sep 12, 2017 •

edited

Loading

erquhart Sep 15, 2017

medfreeman Sep 16, 2017 •

edited

Loading

erquhart Sep 15, 2017

medfreeman Sep 16, 2017

erquhart commented Sep 15, 2017 •

edited

Loading

medfreeman commented Sep 16, 2017

erquhart commented Sep 19, 2017

erquhart Sep 19, 2017

erquhart Sep 19, 2017

erquhart Sep 19, 2017

erquhart Sep 19, 2017

erquhart Sep 19, 2017

erquhart Sep 19, 2017

erquhart commented Feb 28, 2018

Add a displayField property to the relation widget #594

Add a displayField property to the relation widget #594

Conversation

medfreeman commented Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

medfreeman Sep 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erquhart commented Sep 15, 2017 • edited Loading

medfreeman commented Sep 16, 2017

erquhart commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erquhart commented Feb 28, 2018

Add a `displayField` property to the `relation` widget #594

Add a `displayField` property to the `relation` widget #594

medfreeman commented Sep 12, 2017 •

edited

Loading

medfreeman Sep 16, 2017 •

edited

Loading

erquhart commented Sep 15, 2017 •

edited

Loading