Add Tutorials #38

wgranger · 2017-09-29T18:47:47Z

This appears to be functioning well. The only issue I'm having is with the handleUnmount function on the tutorial component displaying the errors below when it hits the projectPreferences.save(); line, yet I'm not quite sure how to fix it, which is why this is RFC.

Aside from that, this should contain all the styles and components necessary to display tutorials.

wgranger · 2017-10-01T14:49:44Z

lib/components/tutorial.js

+  }
+};
+
+Tutorial.start = function (TutorialComponent, tutorial, user, preferences) {


TutorialComponent is an example of where the binding seems a bit odd in this component. You can see where this is being used in PFE here. I think it's a bit odd to have static methods in the tutorial component that refer to this, but perhaps there was a reason for this?

That said, my use case isn't the same as the binding example from the classifier example above, so I'm unable to access this in such a way.

Static methods should be pure functions, in my opinion, so I think you're right to explicitly pass TutorialComponent as the first argument.

The PFE code makes start look like an instance method, which can't be right because the component instance is created when start runs.

Yeah, it's a bit odd how the Tutorial component operates in PFE.

eatyourgreens · 2017-10-02T10:21:03Z

FWIW, the 422 error on save tells me that either a required parameter is missing from the POST request, or the value of one of the request parameters is in an unexpected format.

I find those 422 errors tricky to debug, because the error doesn't explicitly tell you what Panoptes was expecting (but didn't get.)

eatyourgreens · 2017-10-02T10:24:21Z

Forgot to add: if you look at the network request that gives the 422 error, the response from the API might be more helpful than the error message generated by the API client.

eatyourgreens · 2017-10-02T14:29:50Z

Should the generated JS files in lib be included in pull requests? It seems to generate a lot of noise in the PR changes.

eatyourgreens · 2017-10-02T14:31:27Z

src/components/tutorial.jsx

+      if (!projectPreferences.preferences.tutorials_completed_at) {
+        projectPreferences.preferences.tutorials_completed_at = {};
+      };
+      projectPreferences.update(`preferences.tutorials_completed_at.${this.props.tutorial.id}`: now);


I think you might be missing {} around the object here, which would explain the error on save.

This appears to be the root of the problem. It's a bit strange how this is constructed with CoffeeScript vs. ES^, but I'll push up a fix.

srallen · 2017-10-02T14:41:50Z

Re: the generated lib files. I'd prefer they weren't because the version script removes the folder, builds the files, and then versions and pushes to github, but I haven't added the folder to the gitignore yet since we're installing via github. I think after this PR and the one I have for the header, I'll add the lib folder to the gitignore and actually publish on npm instead now that more people are contributing and using this now.

wgranger · 2017-10-02T19:29:48Z

Ok, I'll remove the generated lib files for the new components.

wgranger · 2017-10-02T19:44:29Z

Looks like most of the line deletions now are from the package.lock, which should've changed with a few added dependencies.

eatyourgreens · 2017-10-03T13:51:02Z

src/components/tutorial.jsx

+      projectPreferences.update((
+        obj["preferences.tutorials_completed_at." + this.props.tutorial.id] = now,
+        obj
+      ));


The convoluted syntax here makes it hard to understand what's being passed to update.

I think

projectPreferences.update({`preferences.tutorials_completed_at.${this.props.tutorial.id}`: now});

might be easier to read.

Or you could set the hash of changes separately, if line length is an issue.

const changes = { `preferences.tutorials_completed_at.${this.props.tutorial.id}`: now }; projectPreferences.update(changes);

Agreed with @eatyourgreens on the readability here.

The backend won't accept that sort of string interpolation =>
projectPreferences.update({preferences.tutorials_completed_at.${this.props.tutorial.id}: now});

but I'll try the changes variable.

Could you file that as an issue on the API client?

Done zooniverse/panoptes-javascript-client#69

Thanks! The json client readme specifically says that the syntax for update is person.update({'links.pets': [spot.id]}) so that interpolated key should be supported.

eatyourgreens · 2017-10-03T13:53:02Z

I'm still seeing the lib files in the diff for this PR. Annoying but not a big deal, as @srallen says, because they'll be overwritten when the module is published anyway.

wgranger · 2017-10-03T13:56:30Z

Oh, I see. I removed the created lib files for the new components, but it looks like some additions were made to lib/index.js and lib/zooniverse-react-components.css

srallen

There's a unique key prop warning when running the tests. I think the issue might be in the Tutorial component, but I'm not totally sure.

srallen · 2017-10-03T15:37:33Z

src/components/step-through.jsx

+          <button
+            type="button"
+            className="step-through-direction step-through-next"
+            aria-label="Next step" title="Next"


Can you put title on to its own line?

srallen · 2017-10-03T15:40:05Z

src/components/step-through.jsx

+    } else {
+      const allSteps = Array.from(Array(childrenCount).keys());
+      return (
+        <div className="step-through-controls" style={{position: 'relative'}}>


Let's move this inline style into the class.

srallen · 2017-10-03T15:42:13Z

src/components/tutorial.jsx

+            <MediaCard key={step._key} className="tutorial-step" src={source}>
+              <Markdown>{step.content}</Markdown>
+              <hr />
+              <p style={{ textAlign: 'center' }}>


Can this inline style be moved into a class?

srallen · 2017-10-03T15:45:31Z

src/css/common.styl

+MAIN_HIGHLIGHT = #43bbfd
+SECOND_HIGHLIGHT = #38b978
+
+$reset-button


There's already a reset.styl file that includes the button reset.

srallen · 2017-10-03T15:45:48Z

src/css/common.styl

@@ -0,0 +1,31 @@
+MAIN_HIGHLIGHT = #43bbfd


I've added a colors.styl file coming in the add-header branch

srallen · 2017-10-03T15:47:59Z

src/css/zooniverse-react-components.styl

@@ -6,3 +6,7 @@
 @require './paginator'
 @require './zoo-footer'
 @require './admin-layout-indicator'
+@require './tutorial'


Just a heads up I also changed the import to import using a wildcard in add-header branch. Let's merge that branch first and rebase this branch off those organizational changes.

srallen · 2017-10-03T15:52:07Z

src/css/modal.styl

@@ -0,0 +1,59 @@
+.modal-form-trigger


Is this class used by the tutorial? I doubt it since the child class is something I think is used in the current PFE header... Let's not move unnecessary styles into this.

srallen · 2017-10-03T15:59:13Z

src/components/step-through.jsx

+  }
+
+  handleScroll() {
+    const reactSwipeNode = ReactDOM.findDOMNode(this.swiper);


Use this.swiper directly. findDOMNode is eventually going to be deprecated.

srallen · 2017-10-03T16:00:40Z

src/components/tutorial.jsx

+      projectPreferences.update((
+        obj["preferences.tutorials_completed_at." + this.props.tutorial.id] = now,
+        obj
+      ));


Agreed with @eatyourgreens on the readability here.

wgranger · 2017-10-10T20:27:09Z

This has been updated and rebased from the recent header merge. I think all should be set. I'm only hesitant about the package-lock.json since so many lines appear to be missing.

srallen · 2017-10-11T16:17:53Z

@wgranger Hm, not sure why so many lines would change with the lock file. I'm guessing there was a merge conflict because of the grommet version bump that came in with the header? Is this a regenerated lock file?

srallen

I have a minor comment about file mocks in the tests. This looks close to being ready for merge. Could you update the changelog.md file too? You'll see in the file what the style is for commenting on what the update is.

srallen · 2017-10-11T16:23:10Z

test/media-card.test.js

+import MediaCard from '../src/components/media-card';
+
+const imageSrc = 'https://www.test.png';
+const videoSrc = 'https://www.test.mp4';


Could you make these just 'test.png' and 'test.mp4'? The mix of the url and file extension is a bit odd and potentially confusing (The file extensions are in the place of what would be the top level domain in a URL).

Makes sense.

wgranger · 2017-10-12T16:59:11Z

Yes, this is a regenerated lock file.

srallen

It should be noted that the implementation of the Tutorial component here won't work in PFE. PFE is written to pass the Geordi client as a prop. Since Geordi will be deprecated, logging the tutorial click will have to be done differently.

Some modification might also need to be made to make it work with translations.

wgranger assigned srallen Sep 29, 2017

wgranger commented Oct 1, 2017

View reviewed changes

eatyourgreens reviewed Oct 2, 2017

View reviewed changes

eatyourgreens reviewed Oct 3, 2017

View reviewed changes

srallen suggested changes Oct 3, 2017

View reviewed changes

wgranger added 3 commits October 10, 2017 14:18

Functioning Tutorials

e5c2254

Add Tests and Remove Lib Files

2d9fba0

Remove Unused and Inline Styles

6d9bc32

wgranger mentioned this pull request Oct 10, 2017

String Interpolation Issues zooniverse/panoptes-javascript-client#69

Closed

Use Common Styles

4313888

wgranger force-pushed the add-tutorial branch from 000bdbc to 4313888 Compare October 10, 2017 20:24

wgranger changed the title ~~[RFC] Functioning Tutorials~~ Add Tutorials Oct 10, 2017

srallen suggested changes Oct 11, 2017

View reviewed changes

wgranger added 2 commits October 12, 2017 12:02

Change Test Variable Names

861d421

Update Changelog

5ec67fb

srallen approved these changes Oct 16, 2017

View reviewed changes

srallen merged commit 28d28ce into master Oct 16, 2017

srallen deleted the add-tutorial branch October 16, 2017 18:47

Add Tutorials #38

Add Tutorials #38

Conversation

wgranger commented Sep 29, 2017 • edited Loading

wgranger Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens commented Oct 2, 2017

eatyourgreens commented Oct 2, 2017

eatyourgreens commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srallen commented Oct 2, 2017

wgranger commented Oct 2, 2017

wgranger commented Oct 2, 2017

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

eatyourgreens commented Oct 3, 2017

wgranger commented Oct 3, 2017

srallen 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

wgranger commented Oct 10, 2017

srallen commented Oct 11, 2017

srallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgranger commented Oct 12, 2017

srallen left a comment

Choose a reason for hiding this comment

wgranger commented Sep 29, 2017 •

edited

Loading

wgranger Oct 1, 2017 •

edited

Loading