Better relationship display. #3063

webteckie · 2016-06-21T03:07:46Z

Description of changes

This change hopes to provide a better user experience when browsing the relationship display for items. As currently implemented the updated display looks as follows:

Related issues (if any)

#2319

Testing

Please confirm npm run test-all ran successfully.

mxstbr · 2016-06-21T05:46:05Z

LGTM, nice work!

webteckie · 2016-06-21T10:27:11Z

@mxstbr although I think it looks better I also think it can be improved further. For example, I think it lends itself to a single table. Although not sure how easy that would be to do. What do you think?

webteckie · 2016-06-21T10:31:14Z

Bad key press in cell phone closed this unexpectedly...reopened :=) @mxstbr^

webteckie · 2016-06-23T01:35:40Z

@JedWatson @mxstbr this is now looking like this:

As you can see all relationships are now under a single table and the column names are more explicit to match the relationship configuration in a list.

mxstbr · 2016-06-23T10:31:44Z

admin/client/App/screens/Item/components/RelatedItemsList.js

@@ -22,7 +23,9 @@ const RelatedItemsList = React.createClass({
 	getColumns () {
 		const { relationship, refList } = this.props;
 		const columns = refList.expandColumns(refList.defaultColumns);
-		return columns.filter(i => i.path !== relationship.refPath);
+		const refListNameColumn = columns.filter(i => i.path === refList.nameField.path);
+		const refListRelationshipFieldColumn = columns.filter(i => i.path === relationship.refPath);


For clarity, I'd prefer those two to be:

columns.filter((column) => column.something === somethingElse);

(am not too fussed about the parentheses, but why call the variable inside the filter i when it's a column? Makes it harder to understand when skimming the code)

was just following the consistency of whomever did it first :-) will update

mxstbr · 2016-06-23T10:40:27Z

Did a proper code review (should've done previously, my bad!), left a few comments and nitpicks but the new table looks great! 👍 Good work!

webteckie · 2016-06-23T10:58:19Z

@mxstbr thanks for the code review...this is still in-progress as need to still handle nameless relationships. btw, not sure why my other github ID (epihacker) is being used for the commits...very confusing :-)

webteckie · 2016-06-24T02:12:28Z

@JedWatson @mxstbr I now deem this change set complete. I tested as many use cases of relationships as I could think of to make sure all displays fine. The display is now flexible in that it handles named/unnamed relationships, labeled/unlabeled relationships, and different relationship cardinalities. Again, the display also tries to reflect the relationship configuration to make it easier to reason about.

mxstbr · 2016-06-24T05:23:36Z

LGTM from a code perspective, ping @JedWatson who has a bigger overview of the feature set here.

JedWatson · 2016-06-25T13:47:09Z

@webteckie tbh I'm a bit confused about the problem this is solving, and what your solution actually does (vs. how I expected relationships to behave; agreed there are bugs and/or missing features in the current implementation that we need to address!)

In ca use-case with lots of related items coming in from a single relationship, I would think this update (as I understand it) would make it harder to use, esp. for example in a scenario where pagination / etc is involved...

But I don't know that I understand what you've done properly. Can you describe it for me, maybe with some example List definitions to go along with the screenshots of how it now works?

webteckie · 2016-06-25T16:56:12Z

@JedWatson to me the way things are now is a bit difficult to reason about when relationship values exist as well as the display doesn't make sense when you have multiple relationships defined and no, yet, filled. Obviously, this seems to be a subjective thing I guess since you seem to think the current display is adequate :-) As far as showing examples, I think it's best that you pull it down and compare the existing and the proposed with different relationship configurations.

webteckie · 2016-06-26T10:53:56Z

Latest display looks like this...the parent relationship is now always displayed even if no relationship items configured:

webteckie · 2016-06-26T11:02:13Z

@JedWatson not sure pagination is a relevant issue here since the rows in the table always relate to the number of relationship fields configured in the active list. You would never expect that to be a huge number of fields. To see the actual full set of relationship values in the field you still need to go to the parent list (same as the previous display). I tried to go back to take some snapshots of the previous display to compare and can't even do it. For the exact same item above this is what I see:

Doesn't compare! :-)

wmertens · 2016-06-28T18:21:28Z

admin/client/App/screens/Item/components/RelatedItemsList.js

 		if (this.state.err) {
-			return <div className="Relationship">{this.state.err}</div>;
+			tbody = <tr><td>{this.state.err}</td></tr>;
+		} else if (this.state.items && this.state.items.results && this.state.items.results.length) {


This would read a lot easier with

} else { const results = this.state.items && this.state.items.results if (results) { if (results.length) {...} else {...} }

also, making the spinner tbody only in a final else clause prevents unnecessary React.createElement calls.

wmertens · 2016-06-28T18:31:11Z

👍

wmertens · 2016-06-29T13:49:11Z

admin/client/App/screens/Item/components/RelatedItemsList.js

+	},
+	renderRelationshipTableBody () {
+		const loadedItems = this.state.items;
+		const results = this.state.items && this.state.items.results;


const results = loadedItems && loadedItems.results;

wmertens · 2016-07-01T11:19:51Z

admin/client/App/screens/List/components/ItemsTable/ItemsTableRow.js

@@ -38,7 +38,7 @@ const ItemsRow = React.createClass({
 		});
 		// item fields
 		var cells = this.props.columns.map((col, i) => {
-			var ColumnType = Columns[col.type] || Columns.__unrecognised__;
+			var ColumnType = Columns[col.type] || Columns.text;


@JedWatson I had a KS frontend crash due to Columns.__unrecognised__, so I'm happy with this line, but I presume that the idea was to actually define a column type __unrecognised__ so that it would show in a specific way in the UI?

Yeah, there used to be an __unrecognised__ type, which would warn that an implementation was missing. Looks like it was lost - I think we should add it back again, rather than quietly defaulting to text. Having a type without an implementation is an error case, and means something's gone wrong at a really low level.

@JedWatson how would you handle cases where a list does not have a name mapping? In that case, the item ID, which doesn't not have a representative field type shows. That's what the text default is doing here ;=)

just leaving a note here to clarify that this has been fixed in master

wmertens · 2016-07-01T11:37:14Z

@JedWatson LGTM

wmertens · 2016-07-05T11:31:28Z

admin/client/App/screens/Item/components/RelatedItemsList.js

-		return columns.filter(i => i.path !== relationship.refPath);
+		const refListNameColumn = columns.filter(column => column.path === refList.namePath || (column.path === 'id' && refList.namePath === '_id'));
+		const refListRelationshipFieldColumn = columns.filter(column => column.path === relationship.refPath);
+		return [refListNameColumn[0], refListRelationshipFieldColumn[0]];


@webteckie, I'm seeing this second column as undefined, causing problems during render. Thoughts?

So unless the referring field is in the default columns of the referring list, this crashes.

@wmertens probably an issue in the original code as well? I can look into this, but I'm hesitant to spend any more time on this if @JedWatson is not going to merge it :=)

wmertens · 2016-07-05T13:12:12Z

@webteckie I fixed the column issue, split out the crash fixes, and rebased.

wmertens · 2016-07-05T13:27:02Z

@webteckie now that I've had some time to play with it, I do think the information could be presented better. I would prefer a sub-heading per relationship, and then a list with the named links to the referrers, with a simple - referringField: value after it. That way, long names and long many-relationships can use the whole line.

Also, I would prefer if the value were only shown for many-relationships, otherwise you get a lot of repetition.

wmertens · 2016-07-05T14:39:14Z

Pie in the sky: how about, when you click on a relationship, it folds open a quick view of the item, without editing fields? Kind of like the movie detail view in NetFlix

webteckie · 2016-07-06T16:55:58Z

@wmertens

I would prefer a sub-heading per relationship, and then a list with the named links to the referrers, with a simple - referringField: value after it.

it seems like what you describe above is the way things are :-) The reason for this PR is because that doesn't seem to work that well for me :-)

how about, when you click on a relationship, it folds open a quick view of the item, without editing fields?

That could work, I guess. Feel free :-)

webteckie closed this Jun 21, 2016

webteckie reopened this Jun 21, 2016

webteckie added the in progress label Jun 22, 2016

mxstbr reviewed Jun 23, 2016
View reviewed changes

webteckie added needs input and removed in progress needs input labels Jun 24, 2016

wmertens reviewed Jun 28, 2016
View reviewed changes

wmertens reviewed Jun 29, 2016
View reviewed changes

webteckie force-pushed the relationship-display-enhancements branch from fef1eda to 4fb5fc1 Compare June 30, 2016 15:07

webteckie force-pushed the relationship-display-enhancements branch from 4fb5fc1 to d2e387b Compare June 30, 2016 20:42

wmertens reviewed Jul 1, 2016
View reviewed changes

wmertens reviewed Jul 5, 2016
View reviewed changes

Fix some corner case crashes

5ab098a

wmertens force-pushed the relationship-display-enhancements branch from 4d0f9d6 to d0f0fd6 Compare July 5, 2016 13:05

Improve the display of relationship items.

2a80be7

wmertens force-pushed the relationship-display-enhancements branch from d0f0fd6 to 2a80be7 Compare July 5, 2016 13:11

mxstbr mentioned this pull request Jul 14, 2016

Columns.__unrecognised__ is not defined. #3157

Closed

stennie added the enhancement label May 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better relationship display. #3063

Better relationship display. #3063

webteckie commented Jun 21, 2016

mxstbr commented Jun 21, 2016

webteckie commented Jun 21, 2016 •

edited

Loading

webteckie commented Jun 21, 2016

webteckie commented Jun 23, 2016

mxstbr Jun 23, 2016 •

edited

Loading

webteckie Jun 23, 2016

mxstbr commented Jun 23, 2016

webteckie commented Jun 23, 2016

webteckie commented Jun 24, 2016

mxstbr commented Jun 24, 2016

JedWatson commented Jun 25, 2016

webteckie commented Jun 25, 2016 •

edited

Loading

webteckie commented Jun 26, 2016

webteckie commented Jun 26, 2016

wmertens Jun 28, 2016

wmertens commented Jun 28, 2016

wmertens Jun 29, 2016

wmertens Jul 1, 2016

JedWatson Jul 6, 2016

webteckie Jul 6, 2016 •

edited

Loading

JedWatson Jul 22, 2016

wmertens commented Jul 1, 2016

wmertens Jul 5, 2016

wmertens Jul 5, 2016

webteckie Jul 5, 2016

wmertens commented Jul 5, 2016

wmertens commented Jul 5, 2016

wmertens commented Jul 5, 2016

webteckie commented Jul 6, 2016

Better relationship display. #3063

Are you sure you want to change the base?

Better relationship display. #3063

Conversation

webteckie commented Jun 21, 2016

Description of changes

Related issues (if any)

Testing

mxstbr commented Jun 21, 2016

webteckie commented Jun 21, 2016 • edited Loading

webteckie commented Jun 21, 2016

webteckie commented Jun 23, 2016

mxstbr Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxstbr commented Jun 23, 2016

webteckie commented Jun 23, 2016

webteckie commented Jun 24, 2016

mxstbr commented Jun 24, 2016

JedWatson commented Jun 25, 2016

webteckie commented Jun 25, 2016 • edited Loading

webteckie commented Jun 26, 2016

webteckie commented Jun 26, 2016

Choose a reason for hiding this comment

wmertens commented Jun 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webteckie Jul 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmertens commented Jul 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmertens commented Jul 5, 2016

wmertens commented Jul 5, 2016

wmertens commented Jul 5, 2016

webteckie commented Jul 6, 2016

webteckie commented Jun 21, 2016 •

edited

Loading

mxstbr Jun 23, 2016 •

edited

Loading

webteckie commented Jun 25, 2016 •

edited

Loading

webteckie Jul 6, 2016 •

edited

Loading