search

minderlabs / demo

Minder Demo App
1 stars 0 forks source link

Item interface and heterogeneous item list. #5

Closed adamberenzweig closed 7 years ago

adamberenzweig commented 7 years ago

This change is Reviewable

richburdon commented 7 years ago
:lgtm:

Reviewed 6 of 6 files at r1. Review status: all files reviewed at latest revision, 5 unresolved discussions.

_sub/demo/js/common/components/web/item_list.js, line 48 at r1 (raw file):_

            switch (item.type) {
              case 'item':
                return <Item user={ user } item={ item }/>;

Aren't we moving to Item and Task? (or was this your comment about causing too big of a merge? I'm synced so go ahead if you're ready.)

_sub/demo/js/common/components/web/item_list.js, line 81 at r1 (raw file):_


        searchItems(text: $query) {
          type,

How can we generalize this to have vanilla [Search]ItemList just show Searchable (snippet) fields?

sub/demo/js/common/data/schema.js, line 247 at r1 (raw file):

    type: {
      type: GraphQLString,
      resolve: (item) => 'note'

TODO/ISSUE: do we need this separate from the class Type?

_sub/demo/js/common/data/schema.js, line 256 at r1 (raw file):_

    },

    status: {

Are we able to factor out things that are common across Items?

_sub/demo/js/common/data/schema.json, line 325 at r1 (raw file):_

                "ofType": {
                  "kind": "INTERFACE",
                  "name": "ItemInterface",

Does "Interface" add anything as a suffix? (is there a standard?)

Comments from Reviewable

adamberenzweig commented 7 years ago

Review status: all files reviewed at latest revision, 5 unresolved discussions.

_sub/demo/js/common/components/web/item_list.js, line 48 at r1 (raw file):_

Previously, richburdon (Rich Burdon) wrote… > Aren't we moving to Item and Task? (or was this your comment about causing too big of a merge? I'm synced so go ahead if you're ready.) >

yeah I'll do that as a separate clean commit.

_sub/demo/js/common/components/web/item_list.js, line 81 at r1 (raw file):_

Previously, richburdon (Rich Burdon) wrote… > How can we generalize this to have vanilla [Search]ItemList just show Searchable (snippet) fields? >

That could be a different component, with a different query that returned SearchableInterface instead of ItemInterface.

sub/demo/js/common/data/schema.js, line 247 at r1 (raw file):

Previously, richburdon (Rich Burdon) wrote… > TODO/ISSUE: do we need this separate from the class Type? >

for the client-side, yes. If you're talking about the class types from database.js, those are not visible to the client.

Also: see how this is used in ItemList.render() in the switch statement. If I dump the item object at that point, it only has properties from ItemInterface at that point. The type-specific fields aren't available until you're inside the component that exposes them in a fragment, e.g. Note or Item (Task).

There may be a better way to do this, but I haven't figured it out yet.

_sub/demo/js/common/data/schema.js, line 256 at r1 (raw file):_

Previously, richburdon (Rich Burdon) wrote… > Are we able to factor out things that are common across Items? >

There's no type inheritance, and each new type that implements an interface needs to define its own resolve(). Maybe it's easier in graphene?

_sub/demo/js/common/data/schema.json, line 325 at r1 (raw file):_

Previously, richburdon (Rich Burdon) wrote… > Does "Interface" add anything as a suffix? (is there a standard?) >

Just my convention to distinguish from actual Types.

Comments from Reviewable