Use React components for footer and header #3257

prinzdezibel · 2017-11-08T15:21:02Z

This changeset is about using React components for header and footer.
Additionally, the old way of customizing parts of the layout is retained,
namely to use registry's layout structure to decide on a per-workflow
basis which footer & header component is choosen.

If this prooves to be unnecessary, we could eliminate that part
altogether. This changeset is also about ensuring feature-parity with old
system:

                         |      Old:                |   New:
-------------------------+--------------------------+--------------------------
 Replacing components    | Template.replace         |   replaceComponent
    in general           |                          |
-------------------------+--------------------------+--------------------------
 Replacing parts of      | layoutHeader designates  | layoutHeader designates
    layout per workflow  |   Blaze template name    |   React comp's name
                         | layoutFooter designates  | layoutFooter designates
                         |   Blaze template name    |   React comp's name

1) layoutHeader This template was used for most cases (coreLayout). But since coreLayout.js does include the new Components.NavBar React component, it's obsolete 2) checkoutHeader This was a customized version of the header for the checkout workflow. Nowadays the checkout workflow also uses the standard NavBar component. Therefore it's obsolete, too.

Additionally, the old way of customizing parts of the layout is retained, namely to use registry's layout structure to decide on a per-workflow basis which footer & header component is choosen. If this prooves to be unnecessary, we could eliminate that part altogether. This changeset only does ensure feature-parity with old system: | Old: | New: -------------------------+--------------------------+-------------------------- Replacing components | Template.replace | replaceComponent in general | | -------------------------+--------------------------+-------------------------- Replacing parts of | layoutHeader designates | layoutHeader designates layout per workflow | Blaze template name | React comp's name | layoutFooter designates | layoutFooter designates | Blaze template name | React comp's name

mikemurray · 2017-11-08T18:13:05Z

imports/plugins/core/dashboard/register.js

@@ -50,7 +50,7 @@ Reaction.registerPackage({
    enabled: true,
    structure: {
      template: "dashboardPackages",
-      layoutHeader: "layoutHeader",
+      layoutHeader: "NavBar",


Any time these values change, you'll need to write a migration. This goes for all the register.js files modified.

Migrations can be found in /imports/plugins/core/versions.

thanks. I've written a migration for it.

mikemurray · 2017-11-08T18:14:01Z

imports/plugins/core/layout/client/components/coreLayout.js

  return (
    <div className={pageClassName} id="reactionAppContainer">
-      <Components.NavBar />
+
+      { headerComponent && React.createElement(headerComponent, {}) }


No extra space with the {} in JSX templates.

Done. Btw, is this enforced by eslint?

mikemurray · 2017-11-08T18:14:32Z

imports/plugins/core/layout/client/components/footer.js

+import { registerComponent } from "/imports/plugins/core/components/lib";
+import { Reaction } from "/client/api";
+
+const Footer = () =>


Always wrap arrow functions in () or {}.

Done. I'm assuming you mean like this:

const Footer = () => ( <div/> );

mikemurray · 2017-11-08T18:15:38Z

imports/plugins/core/layout/client/templates/layout/admin/admin.html

@@ -21,11 +21,11 @@
      {{> Template.dynamic template=template}}


Do we even need this file anymore? This should no longer be used, as it's been replaced with React components app.js (located in core/ui package) and coreLayout.js.

I think not, as i've stated in the getFooterComponent helper function. I'm going to delete this file.

mikemurray · 2017-11-08T18:16:36Z

imports/plugins/core/layout/client/templates/layout/admin/admin.js

@@ -10,6 +10,7 @@ import ToolbarContainer from "/imports/plugins/core/dashboard/client/containers/
 import Toolbar from "/imports/plugins/core/dashboard/client/components/toolbar";


Do we even need this file anymore? I think all of this has been moved to various react components.

I think not, as i've stated in the getFooterComponent helper function. I'm going to delete this file.

mikemurray · 2017-11-08T18:21:55Z

imports/plugins/included/product-variant/register.js

      layoutFooter: "",
+      // layoutFooter: "Footer", // TODO: decide if it's desirable to display standard footer in product grid for a stock installation


No commented code.

deleted comment. layout is retained as was, e.g. no footer will be displayed.

mikemurray · 2017-11-08T18:29:15Z

imports/plugins/core/layout/client/components/footer.js

@@ -0,0 +1,19 @@
+import React from "react";


Adding a footer requires design direction from @rymorgan since for the longest time, we've not had one displayed. And the question is, should we even be adding visible one now.

Footer should not be displayed anywhere, if it wasn't already before..
Right now it's nothing more than an exemplary placeholder that's available for usage when implementing plugins.

But then, a plugin would have its own requirements on how the Footer is supposed to be rendered. Maybe it's just unnecessary to have it in its own place.

mikemurray · 2017-11-08T18:29:34Z

imports/plugins/core/layout/client/components/footer.js

+
+const Footer = () =>
+  <div className="reaction-navigation-footer footer-default">
+    <nav className="navbar-bottom " role="navigation">


extra space in quote "navbar-bottom "

prinzdezibel · 2017-11-09T13:11:48Z

@mikemurray Changes implemented. Btw, is there a button or something to signal you, when I've done the requested changes? Or how is your workflow regarding review->change->review cycle?

mikemurray · 2017-11-09T18:12:04Z

@prinzdezibel there is no magic ping button other than [at]-ing someone in a comment, they noticing changes to the PR / branch, or notifying directly.

I'll take a look at this soon.

mikemurray

Getting this error on an existing app on the homepage

layoutHeader not registered.
    at _getComponent (app.js:102296)
    at CoreLayout (app.js:61466)
    at modules.js:108189
    at measureLifeCyclePerf (modules.js:107959)
    at ReactCompositeComponentWrapper._constructComponentWithoutOwner (modules.js:108188)
    at ReactCompositeComponentWrapper._constructComponent (modules.js:108163)
    at ReactCompositeComponentWrapper.mountComponent (modules.js:108071)
    at Object.mountComponent (modules.js:100902)
    at ReactCompositeComponentWrapper.performInitialMount (modules.js:108254)
    at ReactCompositeComponentWrapper.mountComponent (modules.js:108141)

prinzdezibel · 2017-11-10T09:51:25Z

@mikemurray Sorry, I've forgotten to also migrate the Shops collection and additionally there were an error with my previous attempt. (using Packages.update(id,..., { validate: false})

I was ensuring that migration was executing properly, but only in RoboMongo. Should have tested it better. Sorry for that.

prinzdezibel · 2017-11-13T13:06:02Z

@mikemurray Hi Mike, maybe you can have another look into it. should work as suspected now.

mikemurray · 2017-11-14T18:08:30Z

@prinzdezibel looking now

mikemurray

Getting the following error:

Uncaught Error: Component checkoutHeader not registered.
    at _getComponent (components.js:92)
    at CoreLayout (coreLayout.js:16)
    at modules.js?hash=1d95b8a00969795e9c25b7e6569113f786616bf2:108189
    at measureLifeCyclePerf (modules.js?hash=1d95b8a00969795e9c25b7e6569113f786616bf2:107959)
    at ReactCompositeComponentWrapper._constructComponentWithoutOwner (modules.js?hash=1d95b8a00969795e9c25b7e6569113f786616bf2:108188)
    at ReactCompositeComponentWrapper._constructComponent (modules.js?hash=1d95b8a00969795e9c25b7e6569113f786616bf2:108163)
    at ReactCompositeComponentWrapper.mountComponent (modules.js?hash=1d95b8a00969795e9c25b7e6569113f786616bf2:108071)
    at Object.mountComponent (modules.js?hash=1d95b8a00969795e9c25b7e6569113f786616bf2:100902)
    at ReactCompositeComponentWrapper.performInitialMount (modules.js?hash=1d95b8a00969795e9c25b7e6569113f786616bf2:108254)
    at ReactCompositeComponentWrapper.mountComponent (modules.js?hash=1d95b8a00969795e9c25b7e6569113f786616bf2:108141)

Checkout header is used, needs to be converted to react. #3255

rymorgan · 2017-11-14T18:41:09Z

@prinzdezibel I don't think we should be working on a footer w/out thinking of what the broader approach to having a footer? Can you outline your plan for adding a footer is in this PR? Are you going to add an empty footer that will make it easier down the line when we're ready to add a footer option for admins?

prinzdezibel · 2017-11-14T20:19:56Z

@rymorgan Right now the Footer is just a placeholder that basically doesn't (read: should not do) anything. As long as the footer was not shown before in any of the available views/routes, it also will not be displayed now. It's purpose is more to serve as hint for plugin developers/ customers on how to override/use this Footer. Albeit the footer is really just a link to the root URL, a.k.a. home. It's not useful to use as off-the-shelf component - really it's more the starting point that's getting copied from the developer into his custom plugin directory and modded there.

Maybe it can serve as base for later plans, when we're going to offer Footer - Declarations through the Admin-UI (if you meant that – not sure), but then again: We're talking about 10 lines of code. Not really something of substance.

prinzdezibel · 2017-11-14T21:28:27Z

After talking to @mikemurray it seems that the migration has failed, because it was locked in db. Now for the PR to work it's absolutely necessary that the migrations run.
After removing the lock, the migration worked as expected.

rymorgan · 2017-11-14T21:54:09Z

@prinzdezibel That makes sense 👍

prinzdezibel · 2017-11-15T21:03:45Z

@mikemurray Please give it another try from a fresh master with this feature branch merged into.
(Reverse migration will not work 100%, because the migration's inverse function isn't lossless. Not sure if I should invest more time into that)

mikemurray

Other than the comments above, I like your updates the navbar for visibility

mikemurray · 2017-11-16T16:26:43Z

imports/plugins/core/ui-navbar/client/containers/navbar.js

@@ -36,4 +37,23 @@ function composer(props, onData) {

 registerComponent("NavBar", NavBar, composeWithTracker(composer));

+registerComponent("NavBarCheckout", (props, context) => {


One component per file. Also, separate the registration of the component and the definition of the component like we do in other places.

mikemurray · 2017-11-16T16:29:20Z

imports/plugins/core/layout/client/components/footer.js

+  <div className="reaction-navigation-footer footer-default">
+    <nav className="navbar-bottom" role="navigation">
+      <div className="row">
+        <a href={Reaction.Router.pathFor("/")}>Home</a>


Remove this link for now. This template is valuable to have from a replaceComponent standpoint, but the link shows up and doesn't look good.

The below is the profile page.

ok, removed the link from the component.

- One component per file - Separate component and hoc container - Remove link from Footer component

prinzdezibel · 2017-11-16T20:33:03Z

@mikemurray Hope that's ok now. Thanks for your time!

mikemurray · 2017-11-16T21:51:46Z

@prinzdezibel looks good but you have some linting issues.

mikemurray · 2017-11-16T23:36:52Z

@prinzdezibel I went ahead and fixed the linting issues. Adding @zenweasel as a reviewer to take it over the line if necessary.

brent-hoover · 2017-11-17T00:45:24Z

@rymorgan
I know some of this discussion happened on the previous PR but just triple checking that we are cool with this change to the checkout header with just the "home" link?

as opposed to this on master

rymorgan · 2017-11-17T00:57:31Z

@zenweasel Yep, we're reverting back to how it was originally. Navigation at checkout is a conversion no-no.

brent-hoover

I saw one React error but have not been able to reproduce it so I removed that comment. Tested with the fixed version of the example-plugin and all works great. Nice work!

prinzdezibel · 2017-11-17T06:50:34Z

@mikemurray thanks for the fixing the linting errors. I'm still not familiar with my new IDE.

ghost · 2017-12-01T11:29:44Z

Previously, I use custom footer template to replace core footer template.

Template.my_custom_footer.replaces("layoutFooter")

Then I updated Reaction it's self with some "fix" release.
Then my footer disappear, cause no more template base footer.

If you guys made some changes that break some thing, please do not put it under "Fixes".
Fix do not mean to break some things(generally.

Peace ✌️

brent-hoover · 2017-12-01T11:47:18Z

@yiwan In the future how do you think we could do a better job of communicating "breaking" changes? We have an ongoing project to replace Blaze components with React which will shortly result in the complete elimination of Blaze.

ghost · 2017-12-01T13:10:39Z

Just give breaking change a Big and Bold styled section.
It would be better not mixing with other bug fix.

P.S.
I saw the plan, switch to React which I think it's cool.

…footer-and-header Use React components for footer and header

prinzdezibel added 2 commits November 8, 2017 11:33

prinzdezibel requested a review from mikemurray November 8, 2017 16:00

mikemurray requested changes Nov 8, 2017

View reviewed changes

Change requests for PR #3257. Code review from Mike Murray

35ed3c3

prinzdezibel assigned mikemurray Nov 9, 2017

Merge branch 'master' into michael-use-react-for-footer-and-header

75c8d15

mikemurray requested changes Nov 9, 2017

View reviewed changes

prinzdezibel added 2 commits November 10, 2017 10:23

Change requests for PR #3257. Code review from Mike Murray

4813a92

Fix merge conflict

310e6ac

prinzdezibel mentioned this pull request Nov 14, 2017

Adapt plugin documentation to match latest changes on reactioncommerce/reaction-docs#346

Merged

mikemurray requested changes Nov 14, 2017

View reviewed changes

prinzdezibel mentioned this pull request Nov 15, 2017

Fix for example plugin reactioncommerce/reaction-example-plugin#23

Merged

Fix: Customized navigation bar during checkout

0c7367c

prinzdezibel mentioned this pull request Nov 15, 2017

Remove unused Blaze templates. #3255

Closed

mikemurray requested changes Nov 16, 2017

View reviewed changes

Fix change requests from Mike Murray

15d9e2b

- One component per file - Separate component and hoc container - Remove link from Footer component

mikemurray previously approved these changes Nov 16, 2017

View reviewed changes

mikemurray added 2 commits November 16, 2017 15:07

fix lint issue

f7b7aff

fix lint issue

6e40ad7

mikemurray dismissed their stale review via 6e40ad7 November 16, 2017 23:07

mikemurray requested a review from brent-hoover November 16, 2017 23:36

mikemurray approved these changes Nov 16, 2017

View reviewed changes

brent-hoover approved these changes Nov 17, 2017

View reviewed changes

spencern changed the base branch from master to release-1.5.9 November 20, 2017 23:46

spencern merged commit 0d8030e into release-1.5.9 Nov 20, 2017

spencern deleted the michael-use-react-for-footer-and-header branch November 20, 2017 23:46

spencern mentioned this pull request Nov 21, 2017

Release 1.5.9 #3333

Merged

Akarshit pushed a commit that referenced this pull request Jan 7, 2018

Change requests for PR #3257. Code review from Mike Murray

3974248

Akarshit pushed a commit that referenced this pull request Jan 7, 2018

Change requests for PR #3257. Code review from Mike Murray

de6079d

Akarshit pushed a commit that referenced this pull request Jan 7, 2018

Merge pull request #3257 from reactioncommerce/michael-use-react-for-…

688b4f2

…footer-and-header Use React components for footer and header

		@@ -10,6 +10,7 @@ import ToolbarContainer from "/imports/plugins/core/dashboard/client/containers/
		import Toolbar from "/imports/plugins/core/dashboard/client/components/toolbar";

		layoutFooter: "",
		// layoutFooter: "Footer", // TODO: decide if it's desirable to display standard footer in product grid for a stock installation

		@@ -36,4 +37,23 @@ function composer(props, onData) {

		registerComponent("NavBar", NavBar, composeWithTracker(composer));

		registerComponent("NavBarCheckout", (props, context) => {

Use React components for footer and header #3257

Use React components for footer and header #3257

Conversation

prinzdezibel commented Nov 8, 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

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

prinzdezibel Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prinzdezibel commented Nov 9, 2017

mikemurray commented Nov 9, 2017

mikemurray left a comment

Choose a reason for hiding this comment

prinzdezibel commented Nov 10, 2017

prinzdezibel commented Nov 13, 2017

mikemurray commented Nov 14, 2017

mikemurray left a comment

Choose a reason for hiding this comment

rymorgan commented Nov 14, 2017 • edited Loading

prinzdezibel commented Nov 14, 2017

prinzdezibel commented Nov 14, 2017

rymorgan commented Nov 14, 2017

prinzdezibel commented Nov 15, 2017 • edited Loading

mikemurray 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

prinzdezibel commented Nov 16, 2017 • edited Loading

mikemurray commented Nov 16, 2017

mikemurray commented Nov 16, 2017

brent-hoover commented Nov 17, 2017 • edited Loading

rymorgan commented Nov 17, 2017

brent-hoover left a comment

Choose a reason for hiding this comment

prinzdezibel commented Nov 17, 2017

ghost commented Dec 1, 2017 • edited by ghost Loading

brent-hoover commented Dec 1, 2017

ghost commented Dec 1, 2017 • edited by ghost Loading

prinzdezibel commented Nov 8, 2017 •

edited

Loading

prinzdezibel Nov 8, 2017 •

edited

Loading

rymorgan commented Nov 14, 2017 •

edited

Loading

prinzdezibel commented Nov 15, 2017 •

edited

Loading

prinzdezibel commented Nov 16, 2017 •

edited

Loading

brent-hoover commented Nov 17, 2017 •

edited

Loading

ghost commented Dec 1, 2017 •

edited by ghost

Loading

ghost commented Dec 1, 2017 •

edited by ghost

Loading