ENYO-5363: Added graphql-example #61

hangyeolryu · 2018-06-20T18:42:41Z

Use Apollo with Enact and Github GraphqQL API

A sample application that demonstrates how to use GraphqQL with Enact components. It uses Apollo Client (GraphqQL client).

Enact-DCO-1.0-Signed-off-by: HanGyeol Ryu [email protected]

rundmt

Made some comments.

rundmt · 2018-06-20T21:00:58Z

graphql-example/src/config.json

@@ -0,0 +1,3 @@
+{
+	"token":"a67285b115df55b4bc1b38"


Is this token a real token or a fake token?

How about setting it to a blank or default value and warning on the screen if the value is not set?

rundmt · 2018-06-20T21:05:40Z

graphql-example/src/App/App.js

+	}
+}
+
+const App =	MoonstoneDecorator(AppBase);


Looks like there's some extra spacing here

rundmt · 2018-06-20T21:08:27Z

graphql-example/src/views/Detail.js

+		<Query query={GET_USER} variables={{login: userId}}>
+			{({loading, data}) => {
+				if (loading) {
+					return (<Panel {...rest}><p>Loading...</p></Panel>);


Can we move Panel above Query? Seems like we're duplicating a lot.

rundmt · 2018-06-20T21:31:08Z

graphql-example/src/App/App.js

+					/>
+					<Detail userId={userId} lists={lists} />
+				</ActivityPanels>
+			</ApolloProvider>);


Lets put the parenthesis on a different line, we have quite a few different styles we should probably stick to one.

rundmt · 2018-06-20T21:31:34Z

graphql-example/src/views/Detail.js

+				if (loading) {
+					return (<Panel {...rest}><p>Loading...</p></Panel>);
+				} else if (!data && !data.user) {
+					return <Panel {...rest}><p>User not found...</p></Panel>;


Should this have parenthesis like the single line above?

rundmt · 2018-06-20T21:33:04Z

graphql-example/src/views/Detail.js

+				} else if (!data && !data.user) {
+					return <Panel {...rest}><p>User not found...</p></Panel>;
+				} else {
+					return (<Panel {...rest}>


In other files we have parenthesis on different lines should we move the component code down a line?

rundmt · 2018-06-20T21:33:17Z

graphql-example/src/views/Detail.js

+							{lists.org && <List list={data.user.organizations.nodes} />}
+							{lists.fol &&  <List list={data.user.followers.nodes} />}
+						</Column>
+					</Panel>);


Same here parenthesis.

rundmt · 2018-06-20T21:33:45Z

graphql-example/src/views/Search.js

+	render: ({onInputChange, onSearch, onRepoSelection, onFolSelection, onOrgSelection, ...rest}) => {
+		delete rest.onUserIdChange;
+		delete rest.onListSelectionChange;
+		return (<Panel {...rest}>


Same here parenthesis.

rundmt · 2018-06-20T23:26:57Z

graphql-example/src/App/App.js

+	constructor (props) {
+		super(props);
+		this.userId = 'haileyr';
+		this.lists = {


These are all initially set to true should these values be passed to Search for the initial state of the FormCheckboxItem.

rundmt · 2018-06-20T23:27:23Z

graphql-example/src/App/App.js

+
+	constructor (props) {
+		super(props);
+		this.userId = 'haileyr';


Should we make this an empty string?

webOS101 · 2018-06-21T17:50:00Z

graphql-example/package.json

+		"pack-p": "enact pack -p",
+		"watch": "enact pack --watch",
+		"clean": "enact clean",
+		"lint": "enact lint .",


This needs to be updated to use --strict

I updated it.

webOS101 · 2018-06-21T18:29:11Z

graphql-example/src/views/Search.js

 		delete rest.onUserIdChange;
 		delete rest.onListSelectionChange;
 		return (<Panel {...rest}>
+			{!apiToken && <Notification open><p>Please set your github token in src/config.json.</p></Notification>}
 			<Header title="Dev checks" type="compact" />
 			<Input placeholder="Github id" onChange={onInputChange} dismissOnEnter />


Maybe the input could be disabled if apiToken is not set?

rundmt

LGTM

webOS101

Almost done!

webOS101 · 2018-07-16T19:14:29Z

graphql-example/src/App/App.js

+
+		this.state = {
+			userId: '',
+			lists: {},


Can you remove the duplication of data between this.lists and this.state.lists? There should only be one 'source of truth'.

webOS101 · 2018-07-16T19:20:28Z

graphql-example/src/components/list.js

+});
+
+export default ListBase;
+export {ListBase as List, ListBase};


Can you remove the exports of the Base version and just rename to List? This pattern is good for the framework, for consistency, but I don't think we need to demonstrate this in app code.

webOS101 · 2018-07-16T19:21:33Z

graphql-example/src/components/list.js

+		userId: PropTypes.string
+	},
+
+	computed: {


There's not any need to have this indirection. You can just directly assign renderItem below.

That way gives a lint error.

I changed it and do not see a lint error.

See my commit.

Also, can you write up a bug report about enact/display-name returning that warning in situations it clearly should not?

webOS101 · 2018-07-16T19:27:43Z

graphql-example/src/views/Detail.js

+			{({loading, data}) => {
+				if (loading) {
+					return <Panel {...rest}><p>Loading...</p></Panel>;
+				} else if (!data && !data.user) {


This should be || not && or else it will throw.

webOS101 · 2018-07-16T19:28:27Z

graphql-example/src/views/Search.js

+				{!apiToken && <Notification open><p>Please set your github token in src/config.json.</p></Notification>}
+				<Header title="Dev checks" type="compact" />
+				<Input placeholder="Github id" onChange={onInputChange} dismissOnEnter />
+				<IconButton onClick={onSearch} small={false} backgroundOpacity="transparent">search</IconButton>


We should not use the pattern of setting boolean props to false.

webOS101 · 2018-07-16T19:29:51Z

graphql-example/src/views/Search.js

+		onInputChange: (ev, props) => {
+			props.onUserIdChange(ev.value);
+		},
+		onRepoSelection: (value, props) => {


Let's be consistent and use ev throughout the handlers.

it's not returning an event. It returns an object with a value. but I will just change it.

webOS101 · 2018-07-16T20:27:41Z

graphql-example/src/components/list.js

@@ -46,5 +46,4 @@ const ListBase = kind({
 	}
 });

-export default ListBase;


Can you apply this same pattern to the other files as well?

Enact-DCO-1.0-Signed-off-by: HanGyeol Ryu <[email protected]>

Enact-DCO-1.0-Signed-off-by: Derek Tor [email protected]

Enact-DCO-1.0-Signed-off-by: Roy Sutton [email protected]

Djspaceg · 2018-07-16T22:07:40Z

pattern-graphql/src/components/List.js

+		return [
+			<Cell key="header" shrink><Divider>Repositories</Divider></Cell>,
+			<Cell
+				component={VirtualList} size={list.length <= 4 ? (60 * list.length) : null}


size would be better on its own line.

Sure would be! Fixed.

Enact-DCO-1.0-Signed-off-by: Roy Sutton [email protected]

viodragon2

See inline comments.

viodragon2 · 2018-08-08T23:11:55Z

pattern-graphql/src/views/Detail.js

+    user(login: $login) {
+      name
+      avatarUrl
+      organizations(first: 10) {


Why limit to 10? Any reason why we don't show more?

No reason. There was an example used first 10. I can put 1-100, so I will change this to 100.

viodragon2 · 2018-08-08T23:12:29Z

pattern-graphql/src/views/Detail.js

+      repositories(first: 10) {
+        nodes {
+          name
+          url


Is this getting used anywhere?

viodragon2 · 2018-08-08T23:12:48Z

pattern-graphql/src/views/Detail.js

+      }
+      followers(first: 10) {
+        nodes {
+          name


name field is not a required field in github user account which can render an empty item. We should look for login field instead. Or maybe both?

viodragon2 · 2018-08-08T23:18:27Z

pattern-graphql/src/views/Detail.js

+
+	render: ({userId, repo, org, fol, ...rest}) => (
+		<Query query={GET_USER} variables={{login: userId}}>
+			{({loading, data}) => {


should we handle error?

which kind of errors?
I put

else if (!data || !data.user) { return <Panel {...rest}><p>User not found...</p></Panel>;

for any erroneous data. I think this is enough for this app. Or you can suggest. :)

I meant request errors. In case github API does not respond or failed to respond, should we treat it differently?

hm ok. I will write something up.

viodragon2 · 2018-08-08T23:34:12Z

pattern-graphql/src/views/Detail.js

+		<Query query={GET_USER} variables={{login: userId}}>
+			{({loading, data}) => {
+				if (loading) {
+					return <Panel {...rest}><p>Loading...</p></Panel>;


Is there any reason why we can't wrap Panel outside Query?

On a side note, would be it possible to have three separate <Query> for each type and render lists accordingly instead of fetching all type every time? I don't know if we can do flexible layout dynamically, but wanted to throw out there.

I tried putting Panel outside Query, but it doesn't work.

The query response size is not that big, so I thought it's more efficient requesting all at once than querying three times.

Why doesn't it work? Doesn't <Query> simply return render prop function to create children? I can't imagine why it would fail.

Regarding fetching data, the response size depends on the user. For example, torvalds has one of the most followers in the world but has only one organization, and when you search for his github info, it takes a while.
Plus, you also need to think about the purpose of this sample. What are we trying to provide to the developers? What are we providing that's not available from Apollo's documentation? Or are we trying to show that we can use other libraries in general, and don't want to dig too deep into GraphQL?

From my understanding, the purpose if this app was me getting used to Enact and also making an Enact sample app using GraphGL. As I worked on this, I realized that GraphGL was not really a library that will be commonly used with Enact. So, I focused on more of me experiencing Enact.

and for the Panel wrapping Query, I tried it again. It fails because for the html for successful response. It should be wrapped in a tag but if I do so, the layout breaks.

viodragon2 · 2018-08-08T23:57:11Z

pattern-graphql/src/views/Search.js

+		return (
+			<Panel {...rest}>
+				{!apiToken && <Notification open><p>Please set your github token in src/config.json.</p></Notification>}
+				<Header title="Dev checks" type="compact" />


What does "Dev checks" title mean?

viodragon2 · 2018-08-09T00:06:06Z

pattern-graphql/src/views/Search.js

+				<Header title="Dev checks" type="compact" />
+				<Input placeholder="Github id" ref={this.userId} onChange={this.onUserIdChange} onKeyUp={this.onKeyUp} />
+				<IconButton onClick={this.onSearch} backgroundOpacity="transparent">search</IconButton>
+				<FormCheckboxItem defaultSelected={repo} onToggle={this.onRepoToggle}>Repositories</FormCheckboxItem>


once defaultSelected is set there's no point updating.

i'm not understanding this part.

viodragon2 · 2018-08-09T00:06:17Z

pattern-graphql/src/views/Search.js

+				<Input placeholder="Github id" ref={this.userId} onChange={this.onUserIdChange} onKeyUp={this.onKeyUp} />
+				<IconButton onClick={this.onSearch} backgroundOpacity="transparent">search</IconButton>
+				<FormCheckboxItem defaultSelected={repo} onToggle={this.onRepoToggle}>Repositories</FormCheckboxItem>
+				<FormCheckboxItem defaultSelected={org} onToggle={this.onOrgToggle}>Organizations</FormCheckboxItem>


defaultSelected is not needed for this one I believe?

viodragon2 · 2018-08-09T00:14:13Z

pattern-graphql/src/views/Detail.js

+		<Query query={GET_USER} variables={{login: userId}}>
+			{({loading, data}) => {
+				if (loading) {
+					return <Panel {...rest}><p>Loading...</p></Panel>;


why not Spinner?

viodragon2 · 2018-08-09T00:44:28Z

pattern-graphql/src/App/App.js

+			<ApolloProvider client={client}>
+				<ActivityPanels {...this.props} index={index} onSelectBreadcrumb={this.handleSelectBreadcrumb}>
+					<Search
+						apiToken={config.token}


I don't think apiToken needs to belong in Search. You should be able to put Notification adjacent to AcitivityPanels as it's displayed in a floating layer.

viodragon2 · 2018-08-09T01:02:17Z

pattern-graphql/README.md

+
+## Setup and use
+1. Get a personal access token for the [GitHub API](https://github.com/settings/tokens/new).
+  - Assign a name to the token and select the "read:org" scope.


Should we mention "read:org" under "admin:org"?

CLAassistant · 2018-08-09T20:44:15Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Hailey Ryu
❌ hangyeolryu

Hailey Ryu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

ryanjduffy

just dismissing the review request

hangyeolryu requested review from webOS101, ryanjduffy and rundmt June 20, 2018 18:42

hangyeolryu force-pushed the graphql-example branch from f8285b3 to 00e9205 Compare June 20, 2018 20:48

rundmt suggested changes Jun 20, 2018

View reviewed changes

webOS101 reviewed Jun 21, 2018

View reviewed changes

hangyeolryu force-pushed the graphql-example branch from 41c1dbb to ccd36e4 Compare June 21, 2018 18:24

webOS101 reviewed Jun 21, 2018

View reviewed changes

rundmt approved these changes Jun 21, 2018

View reviewed changes

lishammel changed the title ~~Added graphql-example~~ ENYO-5363: Added graphql-example Jul 2, 2018

webOS101 suggested changes Jul 16, 2018

View reviewed changes

webOS101 reviewed Jul 16, 2018

View reviewed changes

hangyeolryu force-pushed the graphql-example branch from cd4e4b2 to 3a09562 Compare July 16, 2018 20:30

Hailey Ryu and others added 8 commits July 16, 2018 17:22

Added graphql-example

bd35164

fixes

077eebe

Added notification when token isn't set.

ff7e3d7

Enact-DCO-1.0-Signed-off-by: HanGyeol Ryu <[email protected]>

use lint --strict

e28f6e1

Changed paren styles to be more uniform

07c90a2

Enact-DCO-1.0-Signed-off-by: Derek Tor [email protected]

Some stylistic changes

01e439c

Enact-DCO-1.0-Signed-off-by: Roy Sutton [email protected]

updated as per changes requested.

180145b

Fix file case and remove indirection

b2803f1

Enact-DCO-1.0-Signed-off-by: Roy Sutton [email protected]

hangyeolryu force-pushed the graphql-example branch from 71b9cb6 to fee50c4 Compare July 16, 2018 21:24

Roy Sutton added 2 commits July 16, 2018 17:25

Add to enact-all-samples

280516c

Enact-DCO-1.0-Signed-off-by: Roy Sutton [email protected]

Update Enact version to next

e2f670d

Enact-DCO-1.0-Signed-off-by: Roy Sutton [email protected]

webOS101 force-pushed the graphql-example branch from fee50c4 to e2f670d Compare July 16, 2018 21:28

Fix sample

c1abd18

Enact-DCO-1.0-Signed-off-by: Roy Sutton [email protected]

webOS101 changed the base branch from master to develop July 16, 2018 22:04

Djspaceg reviewed Jul 16, 2018

View reviewed changes

Move prop to own line

5c622ec

Enact-DCO-1.0-Signed-off-by: Roy Sutton [email protected]

webOS101 force-pushed the graphql-example branch from 738eacf to 5c622ec Compare July 16, 2018 22:14

Roy Sutton and others added 6 commits July 16, 2018 19:22

Fix bad merge in package.json

c8c2882

Enact-DCO-1.0-Signed-off-by: Roy Sutton [email protected]

Set repo selected by default.

c7044ef

Allow enter to search.

c512ed8

Show right title of lists .

ce4c74d

Removed list dup.

9eb2d7c

modified

f69220a

viodragon2 suggested changes Aug 9, 2018

View reviewed changes

viodragon2 reviewed Aug 9, 2018

View reviewed changes

improvements :D

67796b1

ryanjduffy reviewed Jun 17, 2021

View reviewed changes

ENYO-5363: Added graphql-example #61

Are you sure you want to change the base?

ENYO-5363: Added graphql-example #61

Conversation

hangyeolryu commented Jun 20, 2018 • edited Loading

Use Apollo with Enact and Github GraphqQL API

rundmt left a 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

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

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

rundmt left a comment

Choose a reason for hiding this comment

webOS101 left a 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

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

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

viodragon2 left a comment • edited Loading

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

hangyeolryu Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

viodragon2 Aug 9, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Aug 9, 2018 • edited Loading

ryanjduffy left a comment

Choose a reason for hiding this comment

hangyeolryu commented Jun 20, 2018 •

edited

Loading

viodragon2 left a comment •

edited

Loading

hangyeolryu Aug 9, 2018 •

edited

Loading

viodragon2 Aug 9, 2018 •

edited

Loading

CLAassistant commented Aug 9, 2018 •

edited

Loading