Simple UI #16

carlosms · 2018-05-08T14:17:34Z

Part of #4.

This is a simple first version for the UI. A few notes:

To keep this PR smaller and focused on the frontend, the following part of the issue description will be done in a different PR:

take care of passing env var to fronted.
At this point, query should always add LIMIT N with N=100 defined though env var.

I didn't include instructions in the readme because it will change a lot once Add CI, docs and fix integration tests #15 is merged. To test this branch start the go server and then run

cd frontend
yarn start

In @dpordomingo's code he is assuming the new frontend code would live in /public. But I find it a bit strange, because the fronted actually has a sub-dir named public for some files. I this PR I propose /frontend, but this can be easily changed.
We discussed about adding redux to prepare for future increasing complexity. The current code is so simple that I preferred to go without it for now. I would suggest to add it once we add several result tabs. But if you think it should be included now in this PR or a new one, let's discuss it.
The styling is so minimalistic that there is none :D
I chose react-table for the results table. As you can see the code is quite simple and it can easily changed to any other option should we need to. Let me know if you are aware of any other better lib.
I didn't forget about the debug text area at the bottom. I left it because it can be quite useful at this stage of the project, and the code is very localized and easy to remove later.

Signed-off-by: Carlos Martín <[email protected]>

New api.js based on https://github.com/src-d/code-annotation/blob/master/src/api/index.js Signed-off-by: Carlos Martín <[email protected]>

dpordomingo · 2018-05-08T14:21:51Z

I'll review the PR later, but I agree with your proposal @carlosms about storing the front sources under a new /frontend dir.
I'll prepare #15 considering it.

smacker · 2018-05-08T14:43:26Z

frontend/src/App.js

@@ -44,6 +44,13 @@ GROUP BY committer_email, month, repo_id`,
  }

  render() {
+    let ok;


wow. why not

const ok = this.state.response && this.state.response.status === 200

?

try-catch isn't very javascript way to do such things.

or even better:

const { response } = this.state; const ok = response && response.status === 200;

smacker

one comment above. In general ok.

Maybe I wouldn't include debug textarea in PR, you can easily see the response in dev tools. But up to you.

👍 for moving step by step. No need redux at this stage.

Signed-off-by: Carlos Martín <[email protected]>

smacker · 2018-05-08T17:08:49Z

btw do I understand right that you are going to implement proper layout, loading and error state in separate PRs? According to the issue, I would expect it.

carlosms · 2018-05-09T08:18:37Z

@smacker:

btw do I understand right that you are going to implement proper layout, loading and error state in separate PRs? According to the issue, I would expect it.

Of course that's needed, but my understanding from the design doc is that issue #4 (alpha 1 milestone) would only include the most basic UI (as is in this PR), and those things you list would be added for alpha 2, new issues.

ping @bzz for clarification.

smacker · 2018-05-09T08:23:59Z

I would better go with simple but complete UI in alpha0. Like there are still only 2 blocks (input and table), but they fill the whole page in the wireframe. Error replaces table as in the wireframe and so on. But yeah. Up to @bzz.

bzz · 2018-05-09T08:36:17Z

For me, it would be totally fine to expect proper error placement and further enhancement of current components as part of the alpha2 as Carlos suggests, for the reason of getting a really minimal alpha1 faster to the staging env.

bzz

LGTM

bzz · 2018-05-09T11:44:12Z

GUI looks great.

BTW if we already have table, may be we do not need to print JSON

dpordomingo

wow! I liked!! And it worked in my PC...

But since the goal of this PR was to start the structure, and we agreed on to try following cat one if possible, I have some doubts and some issues that imho should be addressed.

dpordomingo · 2018-05-09T15:08:29Z

frontend/.eslintrc.js

+  },
+  plugins: ['import', 'react', 'jest', 'prettier'],
+  rules: {
+    'prettier/prettier': 'error',


comparing with cat, why not adding?

'prettier/prettier': ['error', { singleQuote: true, trailingComma: 'es5' }],

that replaces .prettierrc, it doesn't make sense to have both, so I removed it.

dpordomingo · 2018-05-09T15:10:48Z

frontend/.eslintrc.js

+    'import/no-unresolved': 0,
+    'import/extensions': 0,
+    'func-names': 0,
+    'no-plusplus': ['error', { allowForLoopAfterthoughts: true }],


I miss other rules from cat

'no-underscore-dangle': 0, // because of _super 'react/prop-types': 0, 'no-console': ['error', { allow: ['info', 'warn', 'error'] }], // later we will might need wrapper

why not adding them?

Please see the conversation in https://github.com/src-d/company/pull/127

I didn't found agreement about 'no-console' conf; did you?
Are we accepting console things?
Why not using a wrapper as in CAT?

dpordomingo · 2018-05-09T15:14:40Z

frontend/.gitignore

+/node_modules
+
+# testing
+/coverage


really needed for the front? it's already in the main .gitignore

The gitignore is the create-react-app default. If we see this dir as a kind of sub-project I think it makes sense to leave this .gitignore here for frontend-only files.

Plust the rules in the .gitignore in master do not apply here, instead of /node_modules it should be frontend/node_modules, right?

I'm not sure if the folder is like a "subproject"...

I use to like small .gitignore files... with as fewer rules as possible, so I like the idea of avoiding duplicities; anyhow this is more like an opinion, so if you do not share it, feel free to disregard it 🗡️

About node_modules rule:
if it should be in the main .gitignore it should be frontend/node_modules but since it makes sense to be in the frontend/.gitignore it needs to relative to the frontend dir.

dpordomingo · 2018-05-09T15:15:10Z

frontend/.gitignore

+/coverage
+
+# production
+/build


I think it's not needed by the front... it's already in main .gitignore

Let's please keep the conversation in one thread, above.

dpordomingo · 2018-05-09T15:15:43Z

frontend/.gitignore

+/build
+
+# misc
+.DS_Store


all this misc ones, with .DS_Store and .env.* are already in the main .gitignore

dpordomingo · 2018-05-09T15:28:42Z

frontend/public/index.html

+      homescreen on Android. See https://developers.google.com/web/fundamentals/engage-and-retain/web-app-manifest/
+    -->
+    <link rel="manifest" href="%PUBLIC_URL%/manifest.json">
+    <link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">


what if we use the cat favicon instead of the react one?

Done in 3d57d81.

dpordomingo · 2018-05-09T15:31:57Z

frontend/public/manifest.json

@@ -0,0 +1,15 @@
+{
+  "short_name": "React App",


we should use the gitbase-playground values 💃

Done in 3d57d81.

dpordomingo · 2018-05-09T15:37:32Z

frontend/src/index.js

+import 'bootstrap/dist/css/bootstrap.css';
+import 'bootstrap/dist/css/bootstrap-theme.css';
+import './override.less';
+import './index.css';


could we write only .less code, as in cat?

I would skip this and merge as it is. All the machinery to write less is in place, this file can be changed at any moment when we start to write styles.

Done in 3d57d81.

dpordomingo · 2018-05-09T15:45:39Z

frontend/src/api.js

+      if (json.errors) {
+        throw json.errors;
+      }
+      return json;


the data will be always under a data field; why not returning it as done in cat?

Not really, for this project we have data and meta.

dpordomingo · 2018-05-09T15:48:25Z

frontend/src/api.js

@@ -0,0 +1,82 @@
+function checkStatus(resp) {


why not moving this service into its own place?
In cat we used /api package, for example.

What would be the benefit of moving api.js to /api/index.js? I don't have a strong opinion on this, let me know why and we can move it.

I see two reasons;

first one is kindof opinionated: imo the root dir should be populated with as fewer things as possible because it helps to understand better the structure of the code.

second one is generic: now we have in this file the logic (copy-pasted from cat) to connect with the API (business agnostic), and also the "repository-like" code (business related). Splitting both parts properly, and moving business agnostic to a service, and the business logic to a repository would help us to reuse and test better.

Signed-off-by: Carlos Martín <[email protected]>

carlosms · 2018-05-11T10:36:32Z

@dpordomingo please take another look when you have time.

dpordomingo

LGTM

The only missing things I see are the following ones, that imho shouldn't block this merge since they could be addressed in further iterations.

Since you're the maintainer, let me know if I can/should open issues for any of them that you think we could consider in next iterations, and I'll happily do so to avoid forgetting it.

carlosms · 2018-05-11T14:57:01Z

Thank you for your comments. I agree, those details can be discussed in new issues and changed later on without much difficulty. Please fell free to open any new issues.

Let's merge so we can move forward with the building and release tasks.

dpordomingo · 2018-05-16T07:02:07Z

thanks!
issues created #28 and #29

carlosms added 3 commits May 8, 2018 15:31

Add frontend skeleton, using create-react-app

3830b49

Signed-off-by: Carlos Martín <[email protected]>

Add formatting, bootstrap and testing dependencies

46e122e

Signed-off-by: Carlos Martín <[email protected]>

First basic UI

c34032e

New api.js based on https://github.com/src-d/code-annotation/blob/master/src/api/index.js Signed-off-by: Carlos Martín <[email protected]>

carlosms self-assigned this May 8, 2018

carlosms requested review from smacker, dpordomingo and bzz May 8, 2018 14:17

dpordomingo added the new feature label May 8, 2018

smacker reviewed May 8, 2018

View reviewed changes

smacker approved these changes May 8, 2018

View reviewed changes

Add results table

c5eac76

Signed-off-by: Carlos Martín <[email protected]>

carlosms force-pushed the issue-4-simple-ui branch from 9e5c407 to c5eac76 Compare May 8, 2018 15:57

carlosms mentioned this pull request May 8, 2018

Pass env vars from backend to frontend #17

Merged

bzz approved these changes May 9, 2018

View reviewed changes

carlosms mentioned this pull request May 9, 2018

Basic UI improvements #26

Merged

dpordomingo mentioned this pull request May 9, 2018

Create project skeleton #1

Closed

4 tasks

dpordomingo suggested changes May 9, 2018

View reviewed changes

Apply @dpordomingo's feedback

3d57d81

Signed-off-by: Carlos Martín <[email protected]>

dpordomingo approved these changes May 11, 2018 •

edited

Loading

View reviewed changes

dpordomingo approved these changes May 11, 2018

View reviewed changes

carlosms merged commit 62b1756 into src-d:master May 11, 2018

dpordomingo mentioned this pull request May 16, 2018

Forbid "console" funcs #28

Open

2 tasks

dpordomingo mentioned this pull request May 16, 2018

Move "api.js" into its own place #29

Open

Simple UI #16

Simple UI #16

Conversation

carlosms commented May 8, 2018

dpordomingo commented May 8, 2018

Choose a reason for hiding this comment

smacker left a comment

Choose a reason for hiding this comment

smacker commented May 8, 2018

carlosms commented May 9, 2018

smacker commented May 9, 2018

bzz commented May 9, 2018

bzz left a comment

Choose a reason for hiding this comment

bzz commented May 9, 2018

dpordomingo 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

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

carlosms commented May 11, 2018

dpordomingo left a comment

Choose a reason for hiding this comment

carlosms commented May 11, 2018

dpordomingo commented May 16, 2018