Add MarkdownHelp component #58

jelliotartz · 2017-07-19T16:55:28Z

Resolves #57.

Creates a MarkdownzHelp component, based on MarkdownzHelp, with Talk and custom title conditionally available via props.

Multiline Markdown elements (e.g., lists, headers) are currently rendered with template literal syntax.
Avatar image is currently rendered with string literal.

Originally written in a Zooniverse-React-Components PR to address zooniverse/Zooniverse-React-Components#18.

mcbouslog

Few initial comments, and I don't think the package-lock.json should be included in the PR.

mcbouslog · 2017-07-19T17:08:04Z

README.md

@@ -6,6 +6,8 @@ Markdown viewer and editor for the [Zooniverse](https://www.zooniverse.org). Req

 Available on [npm](http://npmjs.com), include as a dependency using `npm install --save markdownz`

+Any default styles can be added to `src/css/markdown-help.css`. Don't forget to add a `require` statement for the css file!
+
 This package contains two publicly accessible components a Markdown viewer and a Markdown editor for Zooniverse-flavored Markdown:


We'll want to describe the Help component similar to how the Viewer and Editor are as follows:

mcbouslog · 2017-07-19T17:11:56Z

README.md

@@ -6,6 +6,8 @@ Markdown viewer and editor for the [Zooniverse](https://www.zooniverse.org). Req

 Available on [npm](http://npmjs.com), include as a dependency using `npm install --save markdownz`

+Any default styles can be added to `src/css/markdown-help.css`. Don't forget to add a `require` statement for the css file!


This isn't quite clear to me if I didn't know what we did. I think there may be two things we'll want to explain, how as a contributor or dev working in the repo we should include styles (currently the first sentence touches on), and how someone using this as a dependency should use the styles (current the second sentence touches on), but I think they could be separate sentences and explained in a little more detail?

eatyourgreens · 2017-07-28T11:06:01Z

Flagging up that zooniverse/Panoptes-Front-End#3958 has changed the markdown help file.

srallen

I have a few comments, plus @eatyourgreens's needs to be addressed.

srallen · 2017-07-28T13:28:56Z

README.md


 ## Usage

 Available on [npm](http://npmjs.com), include as a dependency using `npm install --save markdownz`

-This package contains two publicly accessible components a Markdown viewer and a Markdown editor for Zooniverse-flavored Markdown:
+For the MarkdownHelp component, any default styles can be added to `src/css/markdown-help.css`. Don't forget to add a `require` statement for the css file!


How about we just have a concise statement of about importing the file with the example?

srallen · 2017-07-28T13:28:59Z

README.md

-This package contains two publicly accessible components a Markdown viewer and a Markdown editor for Zooniverse-flavored Markdown:
+For the MarkdownHelp component, any default styles can be added to `src/css/markdown-help.css`. Don't forget to add a `require` statement for the css file!
+
+This package contains three publicly accessible components: a Markdown viewer and a Markdown editor for Zooniverse-flavored Markdown, and a MarkdownHelp component that displays a modal illustrating styles and how to implement them.


This doesn't display a modal. It's just the content. It's up to the dev to decide how to implement.

srallen · 2017-07-28T13:30:11Z

README.md

@@ -24,6 +26,14 @@ import { MarkdownEditor } from 'markdownz';
 <MarkdownEditor rows={20} value="A String of `Markdown`" onChange={this.handleMarkdownChange} />
 ```

+Help:
+
+```jsx


I don't think putting jsx at the top of the file is necessary.

srallen · 2017-07-28T13:32:37Z

src/components/markdown-help.jsx

@@ -0,0 +1,269 @@
+import React from 'react';
+import Markdown from './markdown';
+import simpleAvatar from '../images/simple-avatar.png';


Let's not import an image like this. Use a hosted service, like https://placeholder.com, for a placeholder as the image example instead.

srallen · 2017-07-28T13:33:25Z

src/components/markdown-help.jsx

+          </tr>
+        </tbody>
+      </table>
+      { talk &&


This is a style thing, but can you get rid of the extra spaces after and before the curly braces?

srallen · 2017-07-28T13:38:10Z

src/components/markdown-help.jsx

+  </table>);
+
+const MarkdownHelp = ({ title, talk }) => {
+  const bulletedList = `


Why are there a few sections of content here pulled into separate constant variables? Why not just have the content directly in with the rest of the content below?

The Markdown component was not rendering multi-line content correctly, so I created the separate variables with string interpolation as a workaround (I tried both with and without string interpolation in the Markdown component, no difference in the result). Below are some screen shots showing the difference. Happy to change this if there's a way to render multiline content in the Markdown component, but so far I haven't found anything on Stack Overflow/google.

I don't see any string interpolation happening? The backticks in javascript are template strings. I think that's your issue and it's being used in both. Remove the backticks and the markdown should be interpreted correctly inline.

Sorry, I meant template strings. Removing them does not resolve the issue.

Ah ok, I understand better now. You need to tell javascript that you want line breaks (which in a round about way using template strings was doing). You could just do this instead: {'- item one\n- item two\n- item three'} which is a bit more clear to me what's going on.

srallen · 2017-07-28T13:38:47Z

src/components/markdown-help.jsx

+
+MarkdownHelp.propTypes = {
+  talk: React.PropTypes.bool,
+  title: React.PropTypes.string


I know the original in PFE had this, but why is the title a prop?

I'm not sure why, I made it a prop because it was so in the original PFE. Would you like me to take it out or leave it?

For questions like this, you can look at the git blame information to find out who the original author was and ask the question. I think this might have been @mcbouslog's first work on PFE...

Hey @mcbouslog, should we leave the title prop in here?

srallen · 2017-07-28T13:39:15Z

test/helper.js

@@ -20,6 +20,10 @@ Object.keys(document.defaultView).forEach((property) => {
  }
 });

+// prevent mocha from compiling .png files
+function noop() { return null; }


This should be removed once the image import is removed.

srallen · 2017-07-28T13:41:03Z

test/markdown-help.test.js

+/* eslint-disable func-names, prefer-arrow-callback */
+/* eslint import/no-extraneous-dependencies: ["error", { "devDependencies": true  }] */
+import React from 'react';
+import { mount } from 'enzyme';


Let's use shallow instead of mount. The first answer on this stackoverflow question has a good explanation.

srallen · 2017-07-28T13:42:57Z

test/markdown-help.test.js

+    });
+  });
+
+  describe('custom props', function() {


Let's collapse the two describe blocks into one and just use setProps after the first two that tested the default props to change the props and then include the next two tests.

jelliotartz · 2017-08-02T21:52:36Z

README.md


 Viewer:

-```jsx


Removing this from the readme messed up the code blocks - Adding again as javascript, but let me know if you have something else in mind.

No I forgot that github flavored markdown does syntax highlighting this way. If jsx is supported, that should be fine.

srallen · 2017-08-03T13:47:54Z

~~@jelliotartz Don't forget to incorporate the changes @eatyourgreens mentioned.~~ Ok you got it. Please next time leave a comment that you've made those kind of changes since that wasn't tied to specific inline review. Thanks.

jelliotartz · 2017-08-03T14:19:14Z

Ok, will do! :)

Add MarkdownHelp component.

ea45477

mcbouslog requested changes Jul 19, 2017

View reviewed changes

jelliotartz added 2 commits July 19, 2017 13:09

Remove package-lock, edit README.

dc9fe66

Prevent mocha from compiling .png files.

24117b9

mcbouslog requested review from srallen and mcbouslog July 27, 2017 19:30

eatyourgreens mentioned this pull request Jul 28, 2017

Clarify how to write links in Markdown zooniverse/Panoptes-Front-End#3958

Merged

9 tasks

srallen suggested changes Jul 28, 2017

View reviewed changes

This was referenced Aug 1, 2017

Common MarkdownEditor component with MarkdownHelp zooniverse/pfe-lab#88

Closed

Implement modal with MarkdownHelp component zooniverse/pfe-lab#87

Closed

Fix requested changes.

50714f1

jelliotartz commented Aug 2, 2017

View reviewed changes

Add javascript code block syntax to readme.

a294ee4

jelliotartz and others added 4 commits August 3, 2017 09:20

Display code blocks in readme with jsx highlighting.

2a52200

Fix title prop and image examples

f712582

Remove css file. Fix test

7f02509

Revert build script change

d3d33a8

srallen approved these changes Aug 7, 2017

View reviewed changes

mcbouslog approved these changes Aug 7, 2017

View reviewed changes

srallen merged commit f496ae8 into master Aug 7, 2017

srallen deleted the markdownz-help branch August 7, 2017 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MarkdownHelp component #58

Add MarkdownHelp component #58

jelliotartz commented Jul 19, 2017

mcbouslog left a comment

mcbouslog Jul 19, 2017

mcbouslog Jul 19, 2017

eatyourgreens commented Jul 28, 2017

srallen left a comment

srallen Jul 28, 2017

srallen Jul 28, 2017

srallen Jul 28, 2017

srallen Jul 28, 2017

srallen Jul 28, 2017

srallen Jul 28, 2017

jelliotartz Aug 2, 2017

srallen Aug 2, 2017

jelliotartz Aug 2, 2017

jelliotartz Aug 2, 2017

srallen Aug 2, 2017

srallen Jul 28, 2017

jelliotartz Aug 2, 2017

srallen Aug 2, 2017

jelliotartz Aug 2, 2017

srallen Jul 28, 2017

srallen Jul 28, 2017

srallen Jul 28, 2017

jelliotartz Aug 2, 2017 •

edited

Loading

srallen Aug 3, 2017

srallen commented Aug 3, 2017 •

edited

Loading

jelliotartz commented Aug 3, 2017

		@@ -6,6 +6,8 @@ Markdown viewer and editor for the [Zooniverse](https://www.zooniverse.org). Req

		Available on [npm](http://npmjs.com), include as a dependency using `npm install --save markdownz`

		Any default styles can be added to `src/css/markdown-help.css`. Don't forget to add a `require` statement for the css file!


		Viewer:

		```jsx

Add MarkdownHelp component #58

Add MarkdownHelp component #58

Conversation

jelliotartz commented Jul 19, 2017

mcbouslog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens commented Jul 28, 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

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

jelliotartz Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srallen commented Aug 3, 2017 • edited Loading

jelliotartz commented Aug 3, 2017

jelliotartz Aug 2, 2017 •

edited

Loading

srallen commented Aug 3, 2017 •

edited

Loading