Create an API for registering and replacing React components #2536

jshimko · 2017-07-07T17:30:46Z

This adds and uses an API for registering/replacing React components across the Reaction UI. Most of the front end of Reaction has been registered (and is now replaceable). Registering the admin components will come afterward in a separate PR.

Several new higher order components (HOC's) have been added for common use cases (injecting the current user, checking whether they are admin or owner, etc). More details in the docs (link below).

With the merge of this PR, we will need to discuss as a group what the workflow for adding new components and HOC's looks like so that we make sure to have consistency. One of the hardest parts of all of these updates was that there was not a lot of consistency in how components were structured and a lot of them were not set up in a way that made overrides or customization easy to implement. Hopefully this new pattern will solve that.

For more detail, see preliminary docs PR branch:

https://github.com/reactioncommerce/reaction-docs/blob/components-api/developer/components/API.md

* development: Fix order action button reverting status to processing (#2541)

…reaction into register-component-api * 'register-component-api' of github.com:reactioncommerce/reaction: Fix issue 2535. Transliteration in Safari/IE (#2553) fix 2547: vertically-center navbar link using @navbar-height height variable instead of percentage height (#2551)

Simply import component for tag item in tag list component

…reaction into register-component-api * 'register-component-api' of github.com:reactioncommerce/reaction: Release 1.4.1 (#2583) LingoHub based on development (#2591) Fix Setting product visibility is not reflected in the Product Grid reactively (#2561)

jshimko · 2017-07-25T03:06:15Z

The issue with the MediaItem component reported by @zenweasel has been fixed. Along with that, I turned the SortableItem container into a true factory function so it could be used as a HOC with registerComponent. (HOC's need to be a function that takes a component and returns another component. Previously, SortableItem took both options and a component).

Anyway, all issues discovered by @zenweasel were ironed out with my last few commits and our conversation in Slack. Thanks for the thorough code review!

aaronjudd

Lots of good stuff here. I found very little not working, (media, etc all working now) and overall seems to be solid and ready to go.

i18n doesn't appear to be working (reactively) -> choose a new language and no changes. when you navigate, updates become visible, but this is a bug.

aaronjudd · 2017-07-25T03:07:37Z

.babelrc

@@ -0,0 +1,25 @@
+{


Is there a reason we're moving this back from package.json?

The thinking here was for two reasons. NPM keeps jacking up the formatting and because babelrc is a file specifically for this use. As our package.json continues to get longer, it's nice to not have it be a place for a bunch of different kinds of configs.

true regarding npm jacking up the formatting. I had just moved the babelrc in 1.4 for the same reason -> as the number of aliases grow, the natural spot to maintain them all is in the same place where we load other packages.. my logic: why have our aliases hidden in babelrc... when it doesn't have to go there and we don't need to maintain multiple files..

(but hey-> let's go with this, for now, not a big deal to me, until I get annoyed by aliases I can't find again).

fair enough

aaronjudd

Would like to see i18n working.
Currency doesn't appear to be changing on PDP variants. (same behavior as i18n)
.babelrc reverted. (not a blocker)

aaronjudd

Error opening SMS settings..

meteor.js?hash=814eae5…:934 Error: In template smsSettings, call to `{{> React ... }}` missing `component` argument.
    at Blaze.View.<anonymous> (react-template-helper.js:24)
    at blaze.js?hash=f33d3df…:1934
    at Function.Template._withTemplateInstanceFunc (blaze.js?hash=f33d3df…:3744)
    at blaze.js?hash=f33d3df…:1932```

aaronjudd

Error opening dashboard email panel:

20modules.js?hash=c62f8dd…:85278 Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in. Check the render method of `ReactTable`.
    in ReactTable (created by SortableTable)
    in div (created by SortableTable)
    in SortableTable (created by Reaction(EmailLogs))
    in div (created by Reaction(CardBody))
    in VelocityTransitionGroupChild (created by TransitionGroup)
    in span (created by TransitionGroup)
    in TransitionGroup (created by VelocityTransitionGroup)
    in VelocityTransitionGroup (created by Reaction(CardBody))
    in Reaction(CardBody) (created by Reaction(EmailLogs))
    in div (created by Reaction(Card))
    in Reaction(Card) (created by Reaction(EmailLogs))
    in div (created by Reaction(CardGroup))
    in Reaction(CardGroup) (created by Reaction(EmailLogs))
    in Reaction(EmailLogs) (created by withProps(EmailLogs))
    in withProps(EmailLogs) (created by Tracker(withProps(EmailLogs)))
    in Tracker(withProps(EmailLogs))
printWarning @ modules.js?hash=c62f8dd…:85278

jshimko · 2017-07-25T04:05:10Z

babelrc reverted
Translation and Currency components no longer pure (so should re-render properly again)

brent-hoover · 2017-07-25T04:06:02Z

Yeah I just saw those email/SMS panel errors as well

aaronjudd · 2017-07-25T04:15:40Z

@jshimko might as well update the version to 1.5.0 as well -> this is big enough we're going to want to identify it in dev...

jshimko · 2017-07-25T04:28:37Z

SMS and email settings errors should be fixed.

jshimko · 2017-07-25T04:28:55Z

Version bumped to 1.5.0

aaronjudd

SMS/etc all looks good now. I'm still seeing some funky behavior with i18n and currency, although it is now acting reactively. Just switching languages/currency logged in and logged out, non-update elements start appearing. I have the feeling this should be a new issue, as I'm not sure it's enough to block this migrating to development.

brent-hoover

A couple of style notes and we need to figure out if importing from @reactioncommerce/reaction-components is a local or an NPM import because we are doing it both ways. But I don't see anything here worth holding up the PR for.

brent-hoover · 2017-07-25T03:56:37Z

imports/plugins/core/components/lib/hoc.js

@@ -0,0 +1,77 @@
+import { composeWithTracker } from "./composer";


Import order

brent-hoover · 2017-07-25T03:59:06Z

imports/plugins/core/dashboard/client/containers/packageListContainer.js

@@ -1,10 +1,9 @@
 import React from "react";
+import { composeWithTracker } from "@reactioncommerce/reaction-components";


Import order

brent-hoover · 2017-07-25T04:10:18Z

imports/plugins/core/email/client/containers/emailConfig.js

-    } else {
-      this.setState({ status: "error" });
+    componentWillMount() {
+      const { settings } = this.props;


If we aren't using settings, couldn't we destructure a nested object in one step? (says guy who just recently reread the destructuring docs)

brent-hoover · 2017-07-25T05:34:59Z

imports/plugins/included/notifications/client/containers/notification.js

@@ -0,0 +1,38 @@
+import { compose, withProps } from "recompose";


Generally I don't know if I am favor of renaming these. I found having container in the name makes it easier to find the appropriate file, and no one died from being too explicit.

brent-hoover · 2017-07-25T05:40:41Z

imports/plugins/included/product-variant/containers/gridItemControlsContainer.js

@@ -1,47 +1,50 @@
-import React, { Component } from "react";
+import React, { PureComponent } from "react";


Also, if we are going to remove container from some file names because they are in a container directory we should probably do that everywhere.

Totally agreed @zenweasel. Adding the word "container" to every file name felt really unnecessary to me when they're already in a /container directory. Also, the base component name (without "container" appended) is how they should be referenced with the new API and you should be able to assume that the final component has that name regardless of whether higher order components are being used.

The only reason I didn't do it across the app is because I already edited almost 300 files. The PR had to end somewhere. :)

But yeah, as components get updated to use the new API, my vote is to remove the container from the name and standardize things.

Yeah, I am against renaming these but oh well.

brent-hoover · 2017-07-25T05:52:41Z

imports/plugins/included/product-variant/containers/productGridContainer.js

-          Logger.error(error);
-          throw new Meteor.Error(error);
-        }
+        Meteor.call("products/updateProductPosition", productId, position, tag, error => {


(this is inherited code but since I noticed) Parenthesis around error?

aaronjudd · 2017-07-25T14:55:21Z

@jshimko @zenweasel has some valid style points... but I couldn't take it anymore.. merged to development.

jshimko and others added 30 commits July 7, 2017 12:57

create API for registering and replacing React components

40bcdaa

register most components in ui plugin

e41b77a

Merge branch 'development' into register-component-api

1cf1b9d

Merge branch 'development' into register-component-api

8218d04

* development: Fix order action button reverting status to processing (#2541)

register all button variants separately

e7882f9

delete empty file

6c7678b

cleanup and register checkout components

c21576a

load registered components in router startup

2b0f7ab

Merge branch 'development' into register-component-api

a1bc3ac

register/cleanup NavBar components and add to CoreLayout

3bd8291

start on register/cleanup of Tag components

5cc1225

set default value to Array for hoc arg

d11192a

register/cleanup Notification components

1a4b601

clean up and register accounts components

7c6f5d0

move all registered component usage inside of render methods

2f50b7f

register CartPanel container

99d1093

Merge branch 'development' into register-component-api

02141f4

mapProps -> withProps

2da4cf2

use registeredf components in NavBar

94ad2c9

register TagNav

4fe03d4

fixing loading issues with navbar

e732c57

changed to exports

2936822

export tag nav components and container

dda8569

export all ui components

2c3da27

Simply import component for tag item in tag list component

Fix broken account dropdown

ce7c5c8

fix icon button props

acae345

fix cart panel, drawer, icon

c517174

Fix broken product detail page

00f7145

export popover and popover content

c4d1ff6

jshimko added 4 commits July 24, 2017 21:35

clean up composer

270f8a5

turn SortableItem into a factory function so it can be used as a HOC

39eeba3

separate ProductGridItems from its container

8d52fee

aaronjudd reviewed Jul 25, 2017

View reviewed changes

aaronjudd suggested changes Jul 25, 2017

View reviewed changes

jshimko added 3 commits July 24, 2017 23:52

revert babelrc

f0c890a

depurify the translations component

902da7d

depurify Currency component

80acec0

aaronjudd suggested changes Jul 25, 2017

View reviewed changes

jshimko added 4 commits July 25, 2017 00:20

thank you, magic Meteor imports folder

c1ba603

separate out the Radium HOC from ReactionLayout and depurify it

ee0afdc

fix email settings components

5e01446

bump to 1.5.0

815d8ac

fix npm sources

849b03e

aaronjudd approved these changes Jul 25, 2017

View reviewed changes

brent-hoover reviewed Jul 25, 2017

View reviewed changes

Merge branch 'development' into register-component-api

1de27d4

aaronjudd merged commit d9e6fb4 into development Jul 25, 2017

aaronjudd deleted the register-component-api branch July 25, 2017 14:56

This was referenced Jul 25, 2017

WIP - document new React components API reactioncommerce/reaction-docs#227

Merged

i18n and currency elements not all updating on change #2593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create an API for registering and replacing React components #2536

Create an API for registering and replacing React components #2536

jshimko commented Jul 7, 2017 •

edited

Loading

jshimko commented Jul 25, 2017

aaronjudd left a comment •

edited

Loading

aaronjudd Jul 25, 2017

jshimko Jul 25, 2017

aaronjudd Jul 25, 2017 •

edited

Loading

jshimko Jul 25, 2017

aaronjudd left a comment •

edited

Loading

aaronjudd left a comment

aaronjudd left a comment •

edited

Loading

jshimko commented Jul 25, 2017

brent-hoover commented Jul 25, 2017

aaronjudd commented Jul 25, 2017

jshimko commented Jul 25, 2017

jshimko commented Jul 25, 2017

aaronjudd left a comment

brent-hoover left a comment

brent-hoover Jul 25, 2017

brent-hoover Jul 25, 2017

brent-hoover Jul 25, 2017

brent-hoover Jul 25, 2017

brent-hoover Jul 25, 2017

jshimko Jul 25, 2017

brent-hoover Jul 25, 2017

brent-hoover Jul 25, 2017

aaronjudd commented Jul 25, 2017

		@@ -0,0 +1,77 @@
		import { composeWithTracker } from "./composer";

		@@ -1,10 +1,9 @@
		import React from "react";
		import { composeWithTracker } from "@reactioncommerce/reaction-components";

		@@ -0,0 +1,38 @@
		import { compose, withProps } from "recompose";

		@@ -1,47 +1,50 @@
		import React, { Component } from "react";
		import React, { PureComponent } from "react";

Create an API for registering and replacing React components #2536

Create an API for registering and replacing React components #2536

Conversation

jshimko commented Jul 7, 2017 • edited Loading

jshimko commented Jul 25, 2017

aaronjudd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronjudd Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronjudd left a comment • edited Loading

Choose a reason for hiding this comment

aaronjudd left a comment

Choose a reason for hiding this comment

aaronjudd left a comment • edited Loading

Choose a reason for hiding this comment

jshimko commented Jul 25, 2017

brent-hoover commented Jul 25, 2017

aaronjudd commented Jul 25, 2017

jshimko commented Jul 25, 2017

jshimko commented Jul 25, 2017

aaronjudd left a comment

Choose a reason for hiding this comment

brent-hoover 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

aaronjudd commented Jul 25, 2017

jshimko commented Jul 7, 2017 •

edited

Loading

aaronjudd left a comment •

edited

Loading

aaronjudd Jul 25, 2017 •

edited

Loading

aaronjudd left a comment •

edited

Loading

aaronjudd left a comment •

edited

Loading