Changing extension to use React #6

danqrodney · 2018-06-21T19:41:51Z

No description provided.

calbach

Cool. In general looks good - I've barely done a codelab for React so I haven't reviewed that part too carefully.

calbach · 2018-06-21T22:39:18Z

src/widgets/concept-widget.tsx

+          <thead>
+            <tr>
+              <th>ID</th>
+              <th style={{textAlign: 'left'}}>Name</th>


CSS file is definitely preferred to this duplicative inline styling.

calbach · 2018-06-21T22:40:14Z

src/widgets/concept-widget.tsx

+          </thead>
+
+          <tbody>
+          {this.model.concepts.map(function(concept) {


Should use an ES6 arrow function here as well

calbach · 2018-06-21T22:40:53Z

src/widgets/concept-widget.tsx

+          {this.model.concepts.map(function(concept) {
+            return (
+                <tr key={concept.conceptId} onClick={() =>
+                    selectRow(concept)}


nit: I'd remove that newline, this is breaking up a short callback function

calbach · 2018-06-21T22:41:34Z

src/widgets/concept-widget.tsx

+          </tbody>
+        </table>
+        <input value='Generate code' type='button' onClick={generateCode}
+               disabled={selectedConceptId == null}/>


!selectedConceptId might be more readable and catch a few more edge cases: 0, null, undefined

calbach · 2018-06-21T22:41:59Z

src/index.ts

@@ -13,10 +13,11 @@ import {

 import {AllOfUsConfig} from './config';
 import {ConceptsService} from './services/concepts.service';
-import {ConceptsWidget} from './widgets/concepts-widget';
+// import {ConceptsWidget} from './widgets/concepts-widget';


revert? Or remove widget?

calbach · 2018-06-21T22:43:54Z

src/widgets/concept-widget.tsx

+                 disabled={!this.model.initialized}/>
+          <br/>
+          <input value='Search' type='button' onClick={searchConcepts}
+                 disabled={!this.model.initialized || this.model.query == null


nit: !this.model.query would catch the 2nd and 3rd case

calbach · 2018-06-21T22:45:17Z

src/widgets/concept-widget.tsx

+export class ConceptsWidgetModel extends VDomModel {
+
+  public initialized: boolean;
+  public conceptsLoaded: boolean;


conceptsLoaded and conceptsLoading seem like they should be mutually exclusive and therefore encoding redundant information. Do you need both? If so, maybe rename or doc

Added an enum that handles the three states here (UNSET, LOADING, LOADED.)

calbach · 2018-06-21T22:46:09Z

src/widgets/concept-widget.tsx

+  }
+
+}
+


nit: excess newlines

rushtong · 2018-06-22T11:50:47Z

src/widgets/concept-widget.tsx

+      return null;
+    }
+    const img_src = 'https://vignette.wikia.nocookie.net/epicrapbattlesofhistory/'
+      + 'images/c/c2/Peanut-butter-jelly-time.gif/revision/latest?cb=20141129150614';


I think this is fun, but should we look for something more consistent with AoU styling?

Yes... I don't have such an asset right now. I asked on Slack where the spinner for workbench comes from... I don't see it in our codebase?

Anyway, for now I'm going to leave this in there to remind us to replace it with something. :)

(In general, this UI is a prototype showing what we can do with JupyterLab extensions and React... I don't intend to actually put it in front of researchers in this form.)

OK, I wound up swapping this out for a React CSS-based spinner from loaders.css.

calbach · 2018-06-22T17:55:10Z

Would prefer someone with React experience approves. If we don't have anyone available I will dig into the docs later today and join you in the learning process before re-reviewing.

danqrodney · 2018-06-22T18:30:12Z

I turned on "watch" mode for JupyterLab when running locally... now when you make CSS changes you just have to reload the page to pick them up, and when you make TypeScript changes you just have to run the "build" target instead of "install" (the latter takes much longer.)

calbach

Looks good, mostly questions.

calbach · 2018-06-22T19:16:58Z

src/widgets/concept-widget.tsx

+  LOADED
+}
+
+export class ConceptsWidgetModel extends VDomModel {


I see some of the docs indicating that you should be emitting a change event like this when you update one of these properties:
https://github.com/jupyterlab/jupyterlab/blob/master/packages/launcher/src/index.tsx#L201

If you dispatched events there, it seems like that would avoid a bunch of the explicit update() calls you're doing below. Otherwise I'm not sure there's any real reason for you to be using a Model at all here - do you get something else out of it?

I have a similar question, what does VDomModel get you? Typical usage is to extend React.Component

calbach · 2018-06-22T19:59:22Z

src/widgets/concept-widget.tsx

+          });
+    };
+    return (
+        <div>


So if I understand correctly, we're rerendering the whole react DOM everytime we get an update; so we're just using this as a templating language (without the data binding). I think that's probably fine given our performance needs - just wanted to confirm this was the intent.

I'm less OK with this approach. One of the strengths of React's virtual DOM is to allow React to manage the re-rendering. If some element of state changes inside of a component, then React will know to call render for the specific elements that are touched by that element of state. The approach here subsumes that primary React function and takes control over the whole dom.

I'm revising my critique here. After reviewing this with Mohs, I think I'm wrong - we still are letting React do the rendering (which it will optimize via the virtual DOM) even if we tell it to re-render the whole table.

calbach · 2018-06-22T20:00:05Z

src/widgets/concept-widget.tsx

+          <input value='Search' type='button' onClick={searchConcepts}
+                 disabled={!this.model.initialized || !this.model.query}/>
+          <br/>
+          {this.loadingImage()}


Note: some other approaches to composition are listed here: https://medium.com/dailyjs/techniques-for-decomposing-react-components-e8a1081ef5da

But the approach you have now definitely makes sense if we're not really doing data binding here and using the React rerendering stuff.

calbach · 2018-06-22T20:05:02Z

src/widgets/concept-widget.tsx

+    });
+  }
+
+  protected render(): React.ReactElement<any> {


I couldn't find documentation for this type. Why any here - what is that generic value supposed to represent?

rushtong · 2018-06-23T01:40:49Z

I want to spend some time with this PR, but was having trouble getting it up and running. Will touch base with Dan on Monday to get some help with that. Also, we have several experienced react developers we could pull in. @dmohs might have some bandwidth.

danqrodney · 2018-06-23T01:59:27Z

@rushtong I am OOO next week, so it may have to wait until I get back.

That said: all you should have to do (hopefully) to get this working is run ./project.rb dev-up, and go to http://localhost:8007 in your browser; once you do, when you open a notebook it should open our extension. If that's not happening, please let me know, and I'll try to reproduce the issue.

rushtong · 2018-06-25T14:01:55Z

Thanks @danqrodney. With a fresh checkout, that command gives me:

./project.rb dev-up
Traceback (most recent call last):
	1: from ./project.rb:3:in `<main>'
./project.rb:3:in `require_relative': cannot load such file -- /Users/grushton/develop/aou/jupyterlab-extensions/utils/workbench (LoadError)

I'm wondering if this project needs to be located somewhere relative to workbench, or if there is a command I'm missing for generating the missing content.

rushtong · 2018-06-25T14:06:52Z

Ah ... found it. I needed to run git submodule update --init --recursive before ./project.rb dev-up

rushtong · 2018-06-25T14:43:07Z

src/widgets/concept-widget.tsx

+    );
+  }
+
+  protected conceptsTable(): React.ReactElement<any> {


I would suggest this be a class component on its own, e.g.ConceptTable. Further, I would break out the rows into another class as well. Right now, we're not doing much with the individual row content aside from loading the concept id, but one could imagine adding sorting and filtering at the table level.

rushtong · 2018-06-26T15:54:44Z

If there is an error accessing the configured workspace (404 in the javascript console), there is an infinite spinner. Should have some kind of error display there.

rushtong · 2018-06-26T15:55:46Z

When the widget loads and a search table is displayed, the widget window can be too small for the results and hide the "generate code" button. I think we need a scrollbar for that case.

danqrodney · 2018-07-02T18:48:58Z

Thanks for the comments @rushtong and @calbach.

The VDomModel / VDomRenderer stuff is JupyterLab-specific... it's wiring up React Elements with Phosphor widgets (which are what gets added to the JupyterLab UI). The only documentation I can find about React in JupyterLab is here:

http://jupyterlab.readthedocs.io/en/stable/developer/virtualdom.html

I don't know how to make a React component (rather than just an element) live inside a Phosphor widget in JupyterLab. If you know how, and think it would work better, please let me know. Though after my experience trying to get Angular working, I'm wary of any approach which isn't explicitly supported by the JupyterLab framework.

In the meantime, I'm going to hit pause on this PR -- building this extension was mostly a proof of concept that we could build something that hits our API and does something useful from JupyterLab, with a technology we like. We may well want an extension like this, but it may be significantly different functionally from what's here now -- we're still working out the plan. (And JupyterLab isn't going to be available in Leo in prod until late next week.) So I will start working on other items while we work out the details.

Dan Rodney added 2 commits June 21, 2018 14:40

Trying react

d7c3fc9

Fixing stuff

f133f6b

danqrodney changed the title ~~Trying react~~ Changing extension to use React Jun 21, 2018

danqrodney requested a review from calbach June 21, 2018 21:53

calbach reviewed Jun 21, 2018

View reviewed changes

rushtong reviewed Jun 22, 2018

View reviewed changes

PR comments

114a1a4

Dan Rodney added 2 commits June 22, 2018 13:26

Switching spinner, enabling watch mode

49904bb

Adding comments

eead56d

calbach approved these changes Jun 22, 2018

View reviewed changes

rushtong reviewed Jun 25, 2018

View reviewed changes

Changing extension to use React #6

Are you sure you want to change the base?

Changing extension to use React #6

Conversation

danqrodney commented Jun 21, 2018

calbach 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

calbach commented Jun 22, 2018

danqrodney commented Jun 22, 2018 • edited Loading

calbach 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

rushtong commented Jun 23, 2018

danqrodney commented Jun 23, 2018

rushtong commented Jun 25, 2018

rushtong commented Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

rushtong commented Jun 26, 2018

rushtong commented Jun 26, 2018

danqrodney commented Jul 2, 2018 • edited Loading

danqrodney commented Jun 22, 2018 •

edited

Loading

rushtong commented Jun 25, 2018 •

edited

Loading

danqrodney commented Jul 2, 2018 •

edited

Loading