Che plugin viewer first working model #1

sunilk747 · 2018-12-28T11:19:30Z

What does this PR do?

First commit for che plugin viewer.

skabashnyuk · 2018-12-28T12:27:12Z

@sunilk747 can you confirm that you have all copyrights for all content that you are submitting(images, resources, etc)?
@benoitf I'm not an expert in UI part can you take a look on fonts. Are they potentially compatible with Eclipse organization rules?

skabashnyuk · 2019-01-02T14:10:07Z

@sunilk747 let me know when you want to merge your work?

skabashnyuk · 2019-01-08T13:41:16Z

@evidolob can you take a look. I need some node/UI experts to take a look.

sunilk747 · 2019-01-09T10:48:28Z

@skabashnyuk Feel free to merge this PR.

evidolob · 2019-01-09T13:38:20Z

Hello, quick review feedback:

It seems that you are missed lodash-es dependency, it is used in che-plugin-viewer/src/common, currently I cannot build project.
Can you add node_modules, build folders in .gitignore ? Also after build there are a lot of git untracked images insade src/img folder, should we also add this folder in .gitignore?
Can you push your yarn.lock file, just to have more reelable build environment?

evidolob · 2019-01-09T13:38:50Z

cc @benoitf Can you review this pull request?

benoitf

no copyright
no license in images
fonts should come through node modules
should use typescript as all of the other che projects
lot more comments inline

Dockerfile

benoitf · 2019-01-09T13:45:24Z

Dockerfile

+    yum clean all && \
+    sed -i 's/listen[[:space:]]*80;/listen 8080;/' /etc/nginx/conf.d/default.conf
+
+RUN chmod 777 /var/log/nginx && chmod 777 /var/cache/nginx && chmod 777 /var/run && rm -rf /var/log/nginx/* && rm -rf /var/cache/nginx/*


this run instruction should be merged with previous one (to not duplicate size of layers)

still pending comment

benoitf · 2019-01-09T13:46:30Z

Dockerfile

+    && yarn install && yarn build
+
+EXPOSE 8080
+CMD ["nginx", "-g", "daemon off;"]


if this dockerfile is for production, it should be a multi stage build (yarn tool should not be in production image)

not solved yet. Should be fixed to be approved

README.md

src/img/Logo_Horizontal_Reversed.svg

src/pages/administration/administration.js

src/pages/catalogA/catalogA.js

src/routes.js

src/utils/catalogItemIcon.js

slemeur · 2019-01-09T14:20:44Z

@sunilk747 in eclipse-che/che-plugin-registry#72 I thought you were working on writing it in typescript and you were expecting to get done last week.
What happened?

sunilk747 · 2019-01-09T14:25:52Z

@slemeur No. I guess there is a miscommunication here. Nobody told me to redo the whole thing in typescript. The work that I have been expecting to get done is some UI changes not redoing the whole project in TS.

slemeur · 2019-01-09T14:43:59Z

Yeah that should have been clarified. But I agree that it's sad that it's not in typescript as we are using it in the whole Che stack.
Just discussing for now - Do you have an idea of the amount of work required for that?

sunilk747 · 2019-01-09T15:02:13Z

@slemeur Yeah, it should have been clarified. I was told to reuse this project - https://github.com/jeff-phillips-18/catalog-test-app which is in JS that's why I didn't change anything. We can redo the whole project in typescript + react. Considering it's a small project I guess it can be one in one sprint task.

sunilk747 · 2019-01-28T09:07:46Z

@benoitf Could you please review it again.

sunilk747 · 2019-01-29T05:36:42Z

no copyright
I was not sure what needs to be added that's why I left it as it is. I have added copyright headers in some files let me know if it's correct, I will add it in other files also.

no license in images
Again, which license should I add?

fonts should come through node modules
I am not very familiar with JS/TS development. I have no idea how to do that.

should use typescript as all of the other che projects
Rebuilding the whole project using TS is completely a new task.

lot more comments inline
I have handled most of the review comments. PTAL and let me know if there is anything left that I need to work upon.

sunilk747 · 2019-01-29T11:53:49Z

@benoitf PTAL.

skabashnyuk

removing approval to take a look after all changes will be merged

benoitf · 2019-01-29T13:39:42Z

@sunilk747

these changes are blocker:

no copyright
I was not sure what needs to be added that's why I left it as it is. I have added copyright headers in some files let me know if it's correct, I will add it in other files also.

About copyright, do you own all the code that is there ? if yes, please use Eclipse Public License v2

no license in images
Again, which license should I add?

The question is not what license you should add, but from where coming these images and the license they have, to see if they're matching their use in a EPLv2 project.

fonts should come through node modules
I am not very familiar with JS/TS development. I have no idea how to do that.

I think it's the basis of JS development. Basic idea is to not add any binaries inside the repositories. Only source code. Fonts are dependencies then they should come through npm.

here is a tiny sample with font-awesome but approach is the same with other fonts
https://stackoverflow.com/questions/48103401/how-to-use-font-awesome-5-installed-via-npm

benoitf · 2019-01-29T13:41:56Z

src/app.scss

@@ -0,0 +1,252 @@
+/**


still invalid copyright headers

benoitf · 2019-01-29T13:42:08Z

src/app.js

@@ -0,0 +1,27 @@
+import React from 'react';


missing copyright headers

benoitf · 2019-01-29T13:42:56Z

src/api/pluginsApi.js

+        Red Hat, Inc. - initial implementation
+*/
+
+import axios from 'axios';


name should not use any camelCase
pluginsApi --> plugins-api.js

camel case is not working good across Windows/Linux/Macos usually

benoitf · 2019-01-29T13:43:35Z

src/api/pluginsApi.js

+        let data = [];
+            return await axios.get(process.env.REACT_APP_REGISTRY_URL).then(async response => {
+            for (let i = 0; i < response.data.length; i++) {
+                let url = 'https://che-plugin-registry.openshift.io' + response.data[i].links.self;


registry URL should be a parameter

benoitf · 2019-01-29T13:44:11Z

src/common/helpers.js

@@ -0,0 +1,94 @@
+import * as _ from 'lodash-es';


copyright headers

benoitf · 2019-01-29T13:44:22Z

src/common/helpers.js

+  REJECTED_ACTION
+};
+
+export default helpers;


missing EOF

benoitf · 2019-01-29T13:44:31Z

src/components/catalogItemDetailsDialog.js

@@ -0,0 +1,117 @@
+/* eslint-disable react/no-did-update-set-state */


benoitf · 2019-01-29T13:44:58Z

src/components/catalogItemDetailsDialog.js

+          <div className="catalog-modal__item catalog-modal__description">
+            <div key="desc">
+              <Alert type="info" onDismiss={infoDismissed}>
+                Go to Eclipse che dashboard then select workspace.


Eclipse Che

benoitf · 2019-01-29T13:45:43Z

src/components/catalogView.js

@@ -0,0 +1,627 @@
+/* eslint-disable react/no-did-update-set-state */


--> catalog-view.js
missing headers

benoitf · 2019-01-29T13:46:06Z

src/components/confirmationModal.js

@@ -0,0 +1,80 @@
+import React from 'react';


rename file + headers

benoitf · 2019-01-29T13:46:16Z

src/components/messageDialog.js

@@ -0,0 +1,113 @@
+import React from 'react';


rename file + headers

benoitf · 2019-01-29T13:46:37Z

src/components/nav.js

@@ -0,0 +1,168 @@
+import React from 'react';


copyright headers

benoitf · 2019-01-29T13:47:11Z

src/components/util/listViewUtils.js

@@ -0,0 +1,30 @@
+import * as React from 'react';


rename file + copyright headers

benoitf · 2019-01-29T13:48:48Z

src/pages/catalogC/catalogC.js

+import { fetchCatalogItems } from '../../redux/actions/catalogActions';
+import { helpers } from '../../common/helpers';
+
+class CatalogC extends React.Component {


please use a meaningful name for this class.
CatalogC is not so meaningful
also rename to not use camelCase and add copyright headers

benoitf · 2019-01-29T13:50:09Z

src/utils/categorizeCatalogItems.js

@@ -0,0 +1,233 @@
+import * as _ from 'lodash-es';


why do we need this external lodash library ?

most of the code is available on ES6. (like forEach, map, etc)

benoitf

can't comment on diff as it's binaries:

what is the favicon.ico ? looks like react icon
names of scr1.png and scr2.png are not meaningful. Also I'm unsure if screenshots are required there

sunilk747 · 2019-01-29T17:53:54Z

@sunilk747

these changes are blocker:

no copyright
I was not sure what needs to be added that's why I left it as it is. I have added copyright headers in some files let me know if it's correct, I will add it in other files also.

About copyright, do you own all the code that is there ? if yes, please use Eclipse Public License v2

No. These codes are not mine. I used this project as base for this which is in apache 2.0.

no license in images
Again, which license should I add?

The question is not what license you should add, but from where coming these images and the license they have, to see if they're matching their use in a EPLv2 project.

fonts should come through node modules
I am not very familiar with JS/TS development. I have no idea how to do that.

I think it's the basis of JS development. Basic idea is to not add any binaries inside the repositories. Only source code. Fonts are dependencies then they should come through npm.

here is a tiny sample with font-awesome but approach is the same with other fonts
https://stackoverflow.com/questions/48103401/how-to-use-font-awesome-5-installed-via-npm

skabashnyuk · 2019-02-19T12:29:21Z

@sunilk747 is this pr ready to be reviewed again? As far as I can see not all of @benoitf 's comments answered. Can you describe your vision of this task?

sunilk747 · 2019-02-19T17:11:29Z

@skabashnyuk I was a bit busy with other tasks I have in this sprint. I will try to handle other comments as soon as possible.

sunilk747 mentioned this pull request Dec 28, 2018

[WIP]che-plugin-viewer first working model [DON't MERGE] eclipse-che/che-plugin-registry#72

Closed

skabashnyuk previously approved these changes Jan 2, 2019

View reviewed changes

skabashnyuk requested a review from evidolob January 8, 2019 13:40

sunilk747 force-pushed the che-plugin-viewer branch from 3e49543 to c7dbed3 Compare January 9, 2019 10:47

sunilk747 changed the title ~~[WIP]Che plugin viewer first working model~~ Che plugin viewer first working model Jan 9, 2019

benoitf suggested changes Jan 9, 2019

View reviewed changes

sunilk747 force-pushed the che-plugin-viewer branch from 7967de8 to 7816f51 Compare January 9, 2019 17:53

skabashnyuk mentioned this pull request Jan 10, 2019

Integrate plugin-viewer eclipse-che/che#12373

Closed

sunilk747 force-pushed the che-plugin-viewer branch 2 times, most recently from dffa8b4 to 429dbb2 Compare January 24, 2019 12:41

sunilk747 force-pushed the che-plugin-viewer branch 2 times, most recently from 462a389 to b7b17e3 Compare January 28, 2019 08:54

Che plugin viewer first working model

80a8dd4

sunilk747 force-pushed the che-plugin-viewer branch from b7b17e3 to 80a8dd4 Compare January 28, 2019 09:05

skabashnyuk reviewed Jan 29, 2019

View reviewed changes