Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better relationship display. #3063

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 69 additions & 39 deletions admin/client/App/screens/Item/components/RelatedItemsList.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import { Columns } from 'FieldTypes';
import { Alert, Spinner } from 'elemental';
import { titlecase } from '../../../../utils/string';

const RelatedItemsList = React.createClass({
propTypes: {
Expand All @@ -21,8 +22,10 @@ const RelatedItemsList = React.createClass({
},
getColumns () {
const { relationship, refList } = this.props;
const columns = refList.expandColumns(refList.defaultColumns);
return columns.filter(i => i.path !== relationship.refPath);
return refList.expandColumns([refList.namePath, relationship.refPath]);
},
getColumnType (type) {
return Columns[type] || Columns.text;
},
loadItems () {
const { refList, relatedItemId, relationship } = this.props;
Expand All @@ -45,51 +48,78 @@ const RelatedItemsList = React.createClass({
this.setState({ items });
});
},
renderItems () {
return this.state.items.results.length ? (
<div className="ItemList-wrapper">
<table cellPadding="0" cellSpacing="0" className="Table ItemList">
{this.renderTableCols()}
{this.renderTableHeaders()}
<tbody>
{this.state.items.results.map(this.renderTableRow)}
</tbody>
</table>
</div>
) : (
<h4 className="Relationship__noresults">No related {this.props.refList.plural}</h4>
);
renderRelationshipColumn (item) {
return <td key={'Relationship' + item.id || ''}>{this.props.relationship.label || titlecase(this.props.relationship.path)}</td>;
},
renderTableCols () {
const cols = this.state.columns.map((col) => <col width={col.width} key={col.path} />);
return <colgroup>{cols}</colgroup>;
renderReferenceListColumn (item) {
const listHref = `${Keystone.adminPath}/${this.props.refList.path}`;
return <td key={'Parent' + item.id} className="Relationship__link"><a href={listHref}>{this.props.refList.label}</a></td>;
},
renderTableHeaders () {
const cells = this.state.columns.map((col) => {
return <th key={col.path}>{col.label}</th>;
});
return <thead><tr>{cells}</tr></thead>;
renderReferenceItemColumn (item) {
const column = this.state.columns[0];
let ColumnType = this.getColumnType(column.type);
const linkTo = `${Keystone.adminPath}/${this.props.refList.path}/${item.id}`;
return <ColumnType key={column.path} list={this.props.refList} col={column} data={item} linkTo={linkTo} />;
},
renderReferenceFieldColumn (item) {
const linkTo = `${Keystone.adminPath}/${this.props.refList.path}/${item.id}`;
const linkValue = this.state.columns[1] ? <a href={linkTo}>{this.state.columns[1].label}</a> : null;
return <td key={'Field' + item.id} className="Relationship__link">{linkValue}</td>;
},
renderReferenceFieldValueColumn (item) {
const column = this.state.columns[1];
let ColumnType = this.getColumnType(column.type);
const linkTo = `${Keystone.adminPath}/${this.props.refList.path}/${item.id}`;
return <ColumnType key={column.path} list={this.props.refList} col={column} data={item} linkTo={linkTo} />;
},
renderTableRow (item) {
const cells = this.state.columns.map((col, i) => {
const ColumnType = Columns[col.type] || Columns.__unrecognised__;
const linkTo = !i ? `${Keystone.adminPath}/${this.props.refList.path}/${item.id}` : undefined;
return <ColumnType key={col.path} list={this.props.refList} col={col} data={item} linkTo={linkTo} />;
});
return <tr key={'i' + item.id}>{cells}</tr>;
return (
<tr key={'table-row-item-' + item.id}>{[
this.renderRelationshipColumn(item),
this.renderReferenceListColumn(item),
this.renderReferenceItemColumn(item),
this.renderReferenceFieldColumn(item),
this.renderReferenceFieldValueColumn(item),
]}</tr>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A key of i could be non-unique, thus lead to confusion and unnecessary rerenders from React. Maybe use 'table-row-item-' + item.id or something like that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, keys need only be unique in the current parent, so just using item.id is fine (as long as that is defined).

);
},
render () {
if (this.state.err) {
return <div className="Relationship">{this.state.err}</div>;
}
const listHref = `${Keystone.adminPath}/${this.props.refList.path}`;
renderItems () {
return this.state.items.results.map(this.renderTableRow);
},
renderSpinner () {
return <tr><td><Spinner size="sm" /></td></tr>;
},
renderNoRelationships () {
return (
<div className="Relationship">
<h3 className="Relationship__link"><a href={listHref}>{this.props.refList.label}</a></h3>
{this.state.items ? this.renderItems() : <Spinner size="sm" />}
</div>
<tr>
{this.renderRelationshipColumn({})}
{this.renderReferenceListColumn({})}
<td>None</td>
<td></td>
<td></td>
</tr>
);
},
renderError () {
return <tr><td>{this.state.err}</td></tr>;
},
renderRelationshipTableBody () {
const results = this.state.items && this.state.items.results;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const results = loadedItems && loadedItems.results;

let tbody = null;
if (this.state.err) {
tbody = this.renderError();
} else if (results && results.length) {
tbody = this.renderItems();
} else if (results && !results.length) {
tbody = this.renderNoRelationships();
} else {
tbody = this.renderSpinner();
}
return tbody;
},
render () {
return <tbody className="Relationship">{this.renderRelationshipTableBody()}</tbody>;
},
});

module.exports = RelatedItemsList;
65 changes: 48 additions & 17 deletions admin/client/App/screens/Item/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,28 +71,60 @@ var ItemView = React.createClass({
createIsOpen: visible,
});
},
// Render this items relationships
renderRelationships () {
const { relationships } = this.props.currentList;
renderRelationshipTableCols () {
return (
<colgroup>{[
<col key="Relationship" />,
<col key="Parent" />,
<col key="Item" />,
<col key="Field" />,
<col key="Values" />,
]}</colgroup>
);
},
renderRelationshipTableHeaders () {
return (
<thead><tr>{[
<th key="Relationship">Relationship Name</th>,
<th key="Parent">Reference List</th>,
<th key="Item">Reference Item</th>,
<th key="Field">Reference Field</th>,
<th key="Value">Reference Field Value</th>,
]}</tr></thead>
);
},
renderRelationshipTableBody () {
const currentList = this.props.currentList;
const { relationships } = currentList;
const keys = Object.keys(relationships);
if (!keys.length) return;
return keys.map(key => {
const relationship = relationships[key];
const refList = listsByKey[relationship.ref];
return (
<RelatedItemsList
key={relationship.path}
list={currentList}
refList={refList}
relatedItemId={this.props.params.itemId}
relationship={relationship}
/>
);
});
},
// Render this items relationships
renderRelationships () {
return (
<div className="Relationships">
<Container>
<h2>Relationships</h2>
{keys.map(key => {
const relationship = relationships[key];
const refList = listsByKey[relationship.ref];
return (
<RelatedItemsList
key={relationship.path}
list={this.props.currentList}
refList={refList}
relatedItemId={this.props.params.itemId}
relationship={relationship}
/>
);
})}
<div className="ItemList-wrapper">
<table cellPadding="0" cellSpacing="0" className="Table ItemList">
{this.renderRelationshipTableCols()}
{this.renderRelationshipTableHeaders()}
{this.renderRelationshipTableBody()}
</table>
</div>
</Container>
</div>
);
Expand Down Expand Up @@ -135,7 +167,6 @@ var ItemView = React.createClass({
</div>
);
}

// When we have the data, render the item view with it
return (
<div data-screen-id="item">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@webteckie webteckie Jul 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ;=)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var linkTo = !i ? `${Keystone.adminPath}/${this.props.list.path}/${itemId}` : undefined;
return <ColumnType key={col.path} list={this.props.list} col={col} data={item} linkTo={linkTo} />;
});
Expand Down
7 changes: 5 additions & 2 deletions admin/client/utils/List.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,12 @@ List.prototype.expandSort = function (input) {
else if (path.charAt(0) === '+') {
path = path.substr(1);
}
const field = this.fields[path];
let field = this.fields[path];
if (!field) {
var candidates = Object.keys(this.fields).filter(key => !(this.fields[key].hidden || false));
field = candidates && candidates.length ? this.fields[candidates[0]] : null;
}
if (!field) {
// TODO: Support arbitary document paths
console.warn('Invalid Sort specified:', path);
return;
}
Expand Down
2 changes: 1 addition & 1 deletion fields/types/text/TextColumn.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ var TextColumn = React.createClass({
render () {
const value = this.getValue();
const empty = !value && this.props.linkTo ? true : false;
const className = this.props.col.field.monospace ? 'ItemList__value--monospace' : undefined;
const className = this.props.col.field && this.props.col.field.monospace ? 'ItemList__value--monospace' : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a crash here, too :) good to see this 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're having crashes here, it means something lower level has gone horribly wrong - this check shouldn't be necessary, we should be validating it higher up

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JedWatson this is the crash you get when it applies the text column type instead of the __unrecognized__ one, so it's indeed secondary and probably not be needed once __unrecognized__ is implemented.

return (
<ItemsTableCell>
<ItemsTableValue className={className} href={this.props.linkTo} empty={empty} padded interior field={this.props.col.type}>
Expand Down
6 changes: 6 additions & 0 deletions server/createApp.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ var methodOverride = require('method-override');
var morgan = require('morgan');

var language = require('../lib/middleware/language');
var createComponentRouter = require('./createComponentRouter');

module.exports = function createApp (keystone, express) {

Expand Down Expand Up @@ -117,6 +118,11 @@ module.exports = function createApp (keystone, express) {
keystone.callHook('pre:routes', req, res, next);
});

// Configure React routes
if (keystone.get('react routes')) {
app.use('/', createComponentRouter(keystone.get('react routes')));
}

// Configure application routes
if (typeof keystone.get('routes') === 'function') {
keystone.get('routes')(app);
Expand Down
25 changes: 25 additions & 0 deletions server/createComponentRouter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
var React = require('react');
var renderToString = require('react-dom/server').renderToString;
var ReactRouter = require('react-router');

var match = ReactRouter.match;
var RoutingContext = ReactRouter.RoutingContext;

module.exports = function createComponentRouter (routes) {
return function componentRouter (req, res, next) {
match({ routes: routes, location: req.url },
function (error, redirectLocation, renderProps) {
if (error) return res.status(500).send(error.message);
if (redirectLocation) {
return res.redirect(302, redirectLocation.pathname + redirectLocation.search);
}
if (renderProps) {
return res.render('default', {
content: renderToString(React.createElement(RoutingContext, renderProps)),
});
}
next(null);
}
);
};
};
2 changes: 1 addition & 1 deletion test/e2e/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ update the test suite so that any broken tests pass again. You can run any of t
from keystone's root directory:

Pre-requisites:
- Make sure that you have Firefox(or Chrome) installed. Firefox is the default browser used.
- Make sure that you have Firefox(or Chrome) installed and in the system path. Firefox is the default browser used.
Using Chrome requires specifying a different --env parameter (see below). For any tests below
you may replace the "--env default" parameter with one of the following:

Expand Down
22 changes: 22 additions & 0 deletions test/e2e/models/misc/NamelessRelationship.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
var keystone = require('../../../../index');
var Types = keystone.Field.Types;

var NamelessRelationship = new keystone.List('NamelessRelationship');

NamelessRelationship.add({
fieldA: {
type: Types.Relationship,
ref: 'TargetRelationship',
hidden: true,
},
fieldB: {
type: Types.Relationship,
ref: 'TargetRelationship',
many: true,
},
});

NamelessRelationship.register();
NamelessRelationship.defaultColumns = 'fieldA, fieldB';

module.exports = NamelessRelationship;
7 changes: 6 additions & 1 deletion test/e2e/models/misc/SourceRelationship.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,14 @@ SourceRelationship.add({
type: Types.Relationship,
ref: 'TargetRelationship'
},
fieldB: {
type: Types.Relationship,
ref: 'TargetRelationship',
many: true
},
});

SourceRelationship.register();
SourceRelationship.defaultColumns = 'name, fieldA';
SourceRelationship.defaultColumns = 'name, fieldA, fieldB';

module.exports = SourceRelationship;
19 changes: 18 additions & 1 deletion test/e2e/models/misc/TargetRelationship.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,24 @@ TargetRelationship.add({
TargetRelationship.relationship({
ref: 'SourceRelationship',
refPath: 'fieldA',
path: 'sourceFieldA'
path: 'sourceFieldA',
label: 'Source Relationship A References'
});
TargetRelationship.relationship({
ref: 'SourceRelationship',
refPath: 'fieldB',
path: 'sourceFieldB'
});
TargetRelationship.relationship({
ref: 'NamelessRelationship',
refPath: 'fieldA',
path: 'namelessFieldA',
label: 'Nameless Field A'
});
TargetRelationship.relationship({
ref: 'NamelessRelationship',
refPath: 'fieldB',
path: 'namelessFieldB'
});

TargetRelationship.register();
Expand Down
Loading