Feature/convert jade to jsx by uppal101 · Pull Request #25 · readmeio/api-explorer

uppal101 · 2017-09-08T17:28:26Z

write test cases for each block type
take care of embed iframe condition
Fix bugs on code to make sure they render correctly
Correct styling issue when images are rendered in the right column

…o feature/convert-jade-to-jsx

domharrington · 2017-09-08T17:37:25Z

Can you run npm test in the top level to see what test failures you have? https://circleci.com/gh/readmeio/api-explorer/15?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link Looks like mostly lint issues so far.

Lots of them are from packages/api-explorer-ui/src/lib/emojis.js so you should be able to add this line to the end of this file https://github.com/readmeio/api-explorer/blob/master/packages/api-explorer-ui/.eslintignore to ignore those.

Some of the lint errors are automatically fixable, you can try and run this to make it fix them for you:

./node_modules/.bin/eslint . --fix

I'll wait to review the code until these style issues are fixed! Ask me on here or on Slack if you need advice for how to fix specific issues.

…o feature/convert-jade-to-jsx

domharrington

Rename JSX component files to be TitleCased e.g. api-header.jsx => ApiHeader.jsx
We need to add marked in. Can you do a search on the legacy jade templates and add a TODO in the code everywhere where marked needs to be added?
Tests!
Make Code component be able to switch between tabs

domharrington · 2017-09-08T21:50:25Z

 coverage
+index.js
+emojis.js
+marked.ks


Should this be marked.js?

domharrington · 2017-09-08T21:51:01Z

  if block.data && block.data.body
    div.callout-body
      != marked(block.data.body)
+


This stuff can be removed now right? We dont need to make any changes to the original legacy jade files?

domharrington · 2017-09-08T21:54:22Z

@@ -0,0 +1,101 @@
+0 silly input [ 'test' ]


Can you delete this file and add it to the .gitignore in the top level?

domharrington · 2017-09-08T22:00:05Z

          api: { method },
          swagger: { path },
          body: `
+          [block:textarea]


This is okay for now, but we should probably come up with a better solution for testing out these blocks in future.

domharrington · 2017-09-08T22:02:17Z

@@ -0,0 +1,36 @@
+import uslug from 'uslug';


Can you change this to use require instead of import?

const uslug = require('uslug');

Just to be consistent with the rest

domharrington · 2017-09-08T22:28:38Z

+                  <img src={image.image[0]} alt={image.caption} />
+                </a>
+              </figure>
+              { (image.caption) && (<figcaption> { image.caption } </figcaption>)}


You dont need () around either of these

{ image.caption && <figcaption>{ image.caption }</figcaption> }

domharrington · 2017-09-08T22:29:39Z

+  );
+};
+
+const ImagesBlock = ({ block }) => {


Do we need to support multiple images per image block?

domharrington · 2017-09-08T22:31:34Z

+  const columns = block.data.cols;
+  const rows = block.data.rows;
+
+  const Th = () => {


I always prefer Array.map over for. This can be made shorter by doing:

return columns.map(column =>(<div className="th">{block.data.data[`h-${c}`]}</div>))

columns and rows aren't an array it returns a number

domharrington · 2017-09-08T22:31:51Z

+  };
+
+  const Tr = () => {
+    const tr = [];


Same comment as above ^^

domharrington · 2017-09-08T22:32:53Z

+          {
+            (block.data.data['h-0'] || block.data.data['h-1']) && (
+              <div className="tr">
+                {Th()}


If it's just a function and not a react component, then it needs to be camelCased th(). If you're expecting it to be a react component, then it needs to be instantiated as one:

<Th />

domharrington · 2017-09-12T00:11:17Z

Items from previous review @uppal101 can you check the ones you've now done?

Rename JSX component files to be TitleCased e.g. api-header.jsx => ApiHeader.jsx
We need to add marked in. Can you do a search on the legacy jade templates and add a TODO in the code everywhere where marked needs to be added?
Tests!
Make Code component be able to switch between tabs

domharrington

Really great stuff! I've added a few comments but mostly small stylistic things.

Also can you add .DS_Store to the gitignore and remove the committed one?

domharrington · 2017-09-12T00:15:38Z

@@ -0,0 +1,12 @@
+const React = require('react');
+const { mount } = require('enzyme');


Do you need to use mount everywhere that you are? I think shallow rendering is better if you don't have to mount and you dont have child components.

domharrington · 2017-09-12T00:18:06Z

+                  <a
+                    href=""
+                    onClick={() => {
+                      // eslint-disable-next-line


We still need to make this work 👍

domharrington · 2017-09-12T00:18:53Z

+  );
+};
+
+const Opts = (props) => {


Can we rename this from Opts to be Content or BlockContent or something?

domharrington · 2017-09-12T00:19:55Z

@@ -0,0 +1,113 @@
+import CallOut from './CallOut';


Can you change these from imports to using require() for consistency.

domharrington · 2017-09-12T00:20:30Z

+      case 'textarea':
+        // eslint-disable-next-line react/no-array-index-key
+        return <TextArea key={i} block={block} />;
+      case 'html' :


Can you remove the space between the case and the colon?

case 'html':

Can you do the same for the next ones too?

domharrington · 2017-09-12T00:29:54Z

+const Opts = require('../../src/block-types/Content');
+
+describe('Opts', () => {
+  test('Maps through content to return text area block type', () => {


For this file, i would use a snapshot test instead. This is mostly just a sanity check to make sure the Content component is passing stuff through correctly to the children. To clean up the file i would also move the block content into a fixtures file and read it in using fs.readFileSync(). This keeps the test a little cleaner and means we dont have to have so many long lines like the embed one below. I can help with this if you need.

domharrington · 2017-09-12T00:30:19Z

+    expect(embedInput.find('iframe').prop('src')).toBe('http://jsbin.com/fewilipowi/edit?js,output');
+  });
+
+  xtest('Embed will have src property if iframe is false', () => {


Did you mean to skip this one?

domharrington · 2017-09-12T00:31:02Z

+
+describe('Embed', () => {
+  test('Embed will have src property if iframe is true', () => {
+    const block = { type: 'embed',


Can you fix the indentation on this and the one below?

const block = { type: 'embed', data: { html: false, url: 'http://jsbin.com/fewilipowi/edit?js,output', title: 'JS Bin', favicon: 'http://static.jsbin.com/images/favicon.png', image: 'http://static.jsbin.com/images/logo.png', iframe: true, width: '100%', height: '300px' }, sidebar: undefined };

domharrington · 2017-09-12T00:32:16Z

+
+describe('Image', () => {
+  test('Image has a tag with property href set to url', () => {
+    const block = { type: 'image', data: { images: [{ image: ['https://files.readme.io/924824e-fullsizeoutput_314.jpeg', 'fullsizeoutput_314.jpeg', 640, 1136, '#c8b396'] }] }, sidebar: undefined };


Can you also indent this one on multiple lines?

domharrington · 2017-09-12T00:34:55Z

@@ -0,0 +1,63 @@
+


Can you remove this empty line?

# Conflicts: # packages/api-explorer-ui/.eslintignore # packages/api-explorer-ui/src/Doc.jsx

Refine interface a little bit, remove `configure()` function. Allow passing in `opts` as a second parameter. Possible options: - `correctnewlines` - comes from `project.flags.correctnewlines` - `stripHtml` - passed in in certain cases Outstanding issues: https://github.com/readmeio/api-explorer/issues/29 https://github.com/readmeio/api-explorer/issues/28

…meio/api-explorer into feature/convert-jade-to-jsx

uppal101 added 9 commits September 6, 2017 11:49

Attempt to convert from jade to jsx

ba72965

Merge branch 'master' of https://github.com/readmeio/api-explorer int…

a712324

…o feature/convert-jade-to-jsx

Merge branch 'master' of https://github.com/readmeio/api-explorer int…

14bc63e

…o feature/convert-jade-to-jsx

Refactor if statements and string interpolation

1570f71

Refactor if-else statements

c11c33a

Merge branch 'master' of https://github.com/readmeio/api-explorer int…

8819983

…o feature/convert-jade-to-jsx

Get text area to work

eb3dc84

Image block is working

0c4c197

Fix https://github.com/readmeio/api-explorer/issues/6

4233d9b

uppal101 requested a review from domharrington September 8, 2017 17:28

uppal101 added 3 commits September 8, 2017 12:34

Merge branch 'master' of https://github.com/readmeio/api-explorer int…

1cc60c5

…o feature/convert-jade-to-jsx

Add propTypes and correct lint errors

10942cf

Fix remaining lint issues

a064c57

domharrington reviewed Sep 8, 2017

View reviewed changes

uppal101 added 4 commits September 8, 2017 16:47

Make changes to codes based off comments in PR

505d7c9

Add tests and correct issues from comments in pr

507a459

Refine tests to not show warnings

cad2508

Correct errors for cli

32d4872

domharrington reviewed Sep 12, 2017

View reviewed changes

uppal101 and others added 9 commits September 12, 2017 13:24

Make changes from pr and have code render

3082d02

Correct cli errors

c019873

Switch tabs on code test complete

6223467

Merge branch 'master' into feature/convert-jade-to-jsx

2bb2014

# Conflicts: # packages/api-explorer-ui/.eslintignore # packages/api-explorer-ui/src/Doc.jsx

Run prettier on the code

9639c07

Get markdown rendering in blocks

6d49890

Prettier changes

fd1286e

Update tests and fix lint

5a20895

Dom Harrington and others added 7 commits September 13, 2017 12:49

Add more tests for sanitizer

10e9ff0

Add some more tests and ignoring some other parts

8dd415d

Add more apiheader tests

2d02cff

Add more tests

a658432

Merge branch 'feature/convert-jade-to-jsx' of https://github.com/read…

7a98ab1

…meio/api-explorer into feature/convert-jade-to-jsx

Test reaches 90% coverage

2c3934f

Prettier changes

3d4bfb7

uppal101 merged commit 09d3433 into master Sep 13, 2017

erunion deleted the feature/convert-jade-to-jsx branch July 1, 2019 16:34

		@@ -0,0 +1,12 @@
		const React = require('react');
		const { mount } = require('enzyme');

Conversation

uppal101 commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domharrington commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domharrington left a comment • edited by uppal101 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uppal101 Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domharrington commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domharrington left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

uppal101 commented Sep 8, 2017 •

edited

Loading

domharrington commented Sep 8, 2017 •

edited

Loading

domharrington left a comment •

edited by uppal101

Loading

uppal101 Sep 8, 2017 •

edited

Loading

domharrington commented Sep 12, 2017 •

edited

Loading