Rewrite frontend in React #1273

paulmelnikow · 2017-11-13T00:51:49Z

I rewrote the frontend in React. It's matched feature-for-feature with the current frontend, with only slight changes in the styling. I did not fuss about making the styling identical; the badge popup looks particularly different.

This makes the front end much easier to develop. I'm really looking forward to implementing #701, to which this paves the way.

This makes light use of Next.js, which I'm using for the first time. I like it. It provides webpack config and dev and build tooling, which worked very well and saved a lot of fussing. I also like the way it handles routing, though we're only using one route right now.

Todo:

Test markup modal on rows without previewUri
Review setting of BASE_URI in dev / production
Set up static build and update deploy scripts
Update npm start
Add npm postinstall?
Refactor / make consistent use of CSS

Deferred:

Daniel15 · 2017-11-13T01:40:33Z

frontend/components/badge-examples.js

+const Category = ({ category, examples, baseUri, isProductionBuild, onClick }) => (
+  <div>
+    <h3 id={category.id}>{ category.name }</h3>
+    <table className='badge'><tbody>


Do the ESLint rules not enforce a consistent attribute quotation style? Line 23 uses " but this line uses '. We should use "" everywhere for JSX attributes.

Also, one tag per line please :)

<table className="badge"> <tbody>

Daniel15

Looks pretty good. I've left some comments inline.

This makes light use of Next.js,

Any particular advantages of Next.js over create-react-app? Does Next.js have any runtime cost/overhead, or is it purely a dev tool?

Daniel15 · 2017-11-13T01:41:15Z

frontend/components/badge-examples.js

+  baseUri: PropTypes.string.isRequired,
+  isProductionBuild: PropTypes.bool.isRequired,
+  onClick: PropTypes.func.isRequired,
+};


I feel like we should avoid propTypes (which only does runtime type checking and can't easily be statically analyzed) and instead use Flow. Flow lets us check the typing statically, and supports React very well.

Daniel15 · 2017-11-13T01:42:10Z

frontend/components/dynamic-badge-maker.js

+    url += '&suffix=' + escapeField(this.suffixInput.value);
+    url += '&query=' + this.queryInput.value;
+    url += '&uri=' + escapeField(this.uriInput.value);
+    return url;


What about using an object of key-value pairs for this? Something like:

buildURI({ colorB: this.state.colorInput, prefix: this.state.prefixInput, // ... })

Daniel15 · 2017-11-13T01:43:43Z

frontend/components/dynamic-badge-maker.js

+        </datalist>
+        <input
+          className="short"
+          ref={input => { this.labelInput = input; }}


This is not really the "React way" of doing things. React components should rarely directly touch the raw DOM elements. Instead of doing this, you should use controlled components (https://reactjs.org/docs/forms.html)

For example:

<input className="short" value={this.state.label} onChange={evt => this.setState({label: evt.target.value}) />

Daniel15 · 2017-11-13T01:45:35Z

frontend/components/markup-modal.js

+export default class MarkupModal extends React.Component {
+  constructor (props) {
+    super(props);
+    this.state = {


You can use class properties to set this, rather than doing it in the constructor:

export default class MarkupModal extends React.Component { state = { // ... }; }

Note: This requires the appropriate Babel preset to be enabled (it should be)

Daniel15 · 2017-11-13T01:46:56Z

frontend/components/markup-modal.js

+
+    return (
+      <Modal
+        isOpen={this.state.isOpen}


isOpen={this.props.example !== null} would avoid the need to have isOpen in state.

Setting state based on props is often an anti-pattern, when the value could just be derived from the props directly.

Daniel15 · 2017-11-13T01:51:39Z

frontend/components/usage.js

+    <h2 id="styles">Styles</h2>
+
+    <p>
+      The following styles are available (flat is the default as of Feb 1st 2015):


Remove "as of Feb 1st 2015", not really needed any more.

Daniel15 · 2017-11-13T01:53:37Z

frontend/components/usage.js

+      </tr>
+      <tr>
+        <td><img src={baseUri + "/badge/style-social-green.svg?style=social"} alt="" /></td>
+        <td><code>https://img.shields.io/badge/style-social-green.svg?style=social</code></td>


There's a lot of repetition here. Consider iterating over an array, roughly like:

const styles = [ 'plastic', 'flat', 'flat-square', ...... ]; <table className="badge-img"><tbody> {styles.map(style => ( <tr> <td><img src={baseUri + '/badge/style-' + style + '-green.svg?style=' + style} alt="" /></td> <td><code>https://img.shields.io/badge/style-{style}-green.svg?style={style}</code></td> </tr> ))} </tbody></table>

Daniel15 · 2017-11-13T01:53:54Z

frontend/components/usage.js

+        <td><code>https://img.shields.io/badge/style-flat--square-green.svg?style=flat-square</code></td>
+      </tr>
+      <tr>
+        <td><img src={baseUri + "/badge/style-for--the--badge-green.svg?style=for-the-badge"} alt="" /></td>


Use single quotes for strings (`baseUri + '/badge/......)

Daniel15 · 2017-11-13T01:54:07Z

frontend/components/usage.js

+      <tr><td><code>?logoWidth=40</code></td>
+        <td>Set the horizontal space to give to the logo</td></tr>
+      <tr><td><code>?link=http://left&amp;link=http://right</code></td>
+        <td>Specify what clicking on the left/right of a badge should do (esp. for


Indentation here is a bit weird.

Daniel15 · 2017-11-13T01:54:46Z

pages/index.js

+            isProductionBuild={isProductionBuild} />
+          <a
+            href="https://gratipay.com/Shields/"
+            style={{ textDecoration: 'none', color: 'rgba(0,0,0,0.1)' }}>


Use CSS file for this? CSS files are better for perf. If you do want to switch to using "CSS in JS", consider using a library like Glamor.

paulmelnikow · 2017-11-13T02:02:19Z

Thanks for the review! Will address + respond to these soon.

paulmelnikow

Have addressed most of these. What's left is the CSS, which I'm going to refactor now, and a few items I want to defer which I listed in the top post.

# Conflicts: # package.json

- Support single-server testing and a local dev server (like Next) that is on a different port from the shields server - Refactor config schema With this change, the suggestions work locally in #1273.

# Conflicts: # package-lock.json

paulmelnikow · 2017-11-26T01:12:34Z

Any particular advantages of Next.js over create-react-app? Does Next.js have any runtime cost/overhead, or is it purely a dev tool?

Here's what I've been able to discern:

Routing and link support
Hot loading
Server side rendering
Bundle splitting and efficient loading
No boilerplate
Good support for customizing configuration without additional boilerplate
Easy upgrades. The interface surface between app and Next is small and well defined.
Comes with styled-jsx

It does have a runtime library.

paulmelnikow · 2017-11-26T01:56:22Z

I am not thrilled about the size of the built website. I'd be fine with something like 100k. This is clocking in around 400k. I did some profiling and it seems like this is substantially attributable to Next.js overhead, which is difficult to justify in a site this small. Otherwise I think we'd be between 50 and 100k.

@Daniel15 mentioned CRA and Gatsby which are good suggestions. Let's experiment with those and address in a follow-up.

Daniel15

This looks good to me!

This is clocking in around 400k.

Wow, that's pretty big for a site that's essentially one page. The shields site likely doesn't need many of the features Next provides. create-react-app is probably a good fit, or just manually configure Webpack (which shouldn't be too difficult given the relative simplicity of the site).

Daniel15 · 2017-11-27T01:34:00Z

.travis.yml

@@ -17,7 +17,7 @@ before_script:
 script:
  - npm run lint
  - npm run test:js
-  - npm run build:production
+  - if [ "$TRAVIS_NODE_VERSION" == "6" ]; then echo "Skipping build on Node 6."; else make website; fi


Instead of blacklisting Node 6, what about whitelisting just the one Node version you want to perform the site build on?

Even better, this could be switched to CircleCI and you could use a separate build job for the site, with whichever Node.js version you like.

I guess this is just a temporary transition state though, right?

It seems safer to blacklist Node 6, because if we upgrade beyond Node 8 this would just stop running. Since I added check-node-version I could replace this with a semver expression, which expresses the intent even better.

Yea, it's just temporary until we get Node 8 on the server.

Daniel15 · 2017-11-27T01:35:35Z

frontend/components/footer.js

+      And tell us, we might be able to bring it to you anyway!
+    </p>
+    <p>
+      <object


Why <object> rather than <img>?

It makes embedded links work! Probably that belongs in a Shields FAQ somewhere…

paulmelnikow · 2017-11-28T16:34:27Z

Let’s open new issues for bugs, and track follow-ups here: https://github.com/badges/shields/projects/1

gempain · 2017-12-05T07:32:40Z

Great job @paulmelnikow it's nice to see the effort ! Keep up the great work !

Using fix from mapbox/react-click-to-select#15. From #1273

[WIP] Rewrite frontend in React

5f06776

paulmelnikow added the frontend The Docusaurus app serving the docs site label Nov 13, 2017

paulmelnikow mentioned this pull request Nov 13, 2017

Repository refactoring #1210

Closed

Daniel15 reviewed Nov 13, 2017

View reviewed changes

Daniel15 requested changes Nov 13, 2017

View reviewed changes

paulmelnikow mentioned this pull request Nov 13, 2017

Document the web-content features for badges #953

Closed

paulmelnikow added 10 commits November 13, 2017 17:01

Double quotes in jsx, one tag per line, minor reformatting

4462e1a

Refactor static + dynamic badge creation; use controlled component

aaab2eb

Use class properties

835bf1c

Rm unused ids and reformat

82d34ad

MarkupModal: Remove isOpen from state and rewrite markup rendering

004b50e

propTypes should be static

d218e69

Clean up + modernize network fetch code

1c74e54

Avoid UI jitter during subsequent suggest requests

0d2078a

Singlequote strings

5d6d47b

Reformat / reindent

0707634

paulmelnikow commented Nov 16, 2017

View reviewed changes

paulmelnikow added 3 commits November 17, 2017 19:21

Refactor CSS and replace inline CSS with styled-jsx

d4f6319

Merge branch 'master' into react-frontend

1545512

# Conflicts: # package.json

Configure allowed CORS headers

2e53fba

paulmelnikow mentioned this pull request Nov 21, 2017

Allow configuring allowed CORS origins for suggest #1282

Merged

Fixes for BASE_URL in dev

5e881c9

paulmelnikow mentioned this pull request Nov 21, 2017

[RFC] New API for registering services #963

Merged

paulmelnikow added 2 commits November 22, 2017 15:08

Build frontend in Node 8

89d5779

Clear warnings, fix whitespace, fix dynamic badge maker

46fad74

paulmelnikow added 3 commits November 25, 2017 19:56

Rm unused

b2c4761

PRODUCTION_BUILD -> LONG_CACHE

a9750bc

Merge branch 'master' into react-frontend

4da466b

# Conflicts: # package-lock.json

Update package lock

cd66f0b

paulmelnikow changed the title ~~[WIP] Rewrite frontend in React~~ Rewrite frontend in React Nov 26, 2017

paulmelnikow added 2 commits November 25, 2017 20:42

Clean lint

aead7f9

Try to fix travis in Node 8 + debug Node 6

19a919b

Fix build check on Node 6

4e3d46a

Daniel15 approved these changes Nov 27, 2017

View reviewed changes

paulmelnikow added 2 commits November 27, 2017 17:40

Use check-node-version

0aa7976

Merge branch 'master' into react-frontend

aaa47f0

paulmelnikow merged commit 4b5bf03 into badges:master Nov 28, 2017

paulmelnikow deleted the react-frontend branch November 28, 2017 16:34

tooomm mentioned this pull request Dec 2, 2017

issues with new frontend rewrite #1311

Closed

2 tasks

espadrine mentioned this pull request Dec 3, 2017

Request for reversal of the addition of an HTTP server #1314

Closed

paulmelnikow mentioned this pull request Dec 6, 2017

Fix validateDOMNesting warning #1342

Merged

paulmelnikow added a commit that referenced this pull request Dec 6, 2017

Fix validateDOMNesting warning (#1342)

5527d9d

Using fix from mapbox/react-click-to-select#15. From #1273

espadrine mentioned this pull request Jan 21, 2018

Badges in footer have disappeared | Frontend not up to date #1458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite frontend in React #1273

Rewrite frontend in React #1273

paulmelnikow commented Nov 13, 2017 •

edited

Loading

Daniel15 Nov 13, 2017

Daniel15 left a comment

Daniel15 Nov 13, 2017 •

edited

Loading

Daniel15 Nov 13, 2017

Daniel15 Nov 13, 2017

Daniel15 Nov 13, 2017

Daniel15 Nov 13, 2017

Daniel15 Nov 13, 2017

Daniel15 Nov 13, 2017

Daniel15 Nov 13, 2017

Daniel15 Nov 13, 2017

Daniel15 Nov 13, 2017

paulmelnikow commented Nov 13, 2017

paulmelnikow left a comment

paulmelnikow commented Nov 26, 2017

paulmelnikow commented Nov 26, 2017

Daniel15 left a comment

Daniel15 Nov 27, 2017

paulmelnikow Nov 27, 2017

Daniel15 Nov 27, 2017

paulmelnikow Nov 27, 2017

paulmelnikow commented Nov 28, 2017

gempain commented Dec 5, 2017

Rewrite frontend in React #1273

Rewrite frontend in React #1273

Conversation

paulmelnikow commented Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Daniel15 left a comment

Choose a reason for hiding this comment

Daniel15 Nov 13, 2017 • edited Loading

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

paulmelnikow commented Nov 13, 2017

paulmelnikow left a comment

Choose a reason for hiding this comment

paulmelnikow commented Nov 26, 2017

paulmelnikow commented Nov 26, 2017

Daniel15 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

paulmelnikow commented Nov 28, 2017

gempain commented Dec 5, 2017

paulmelnikow commented Nov 13, 2017 •

edited

Loading

Daniel15 Nov 13, 2017 •

edited

Loading