HOC - Higher order components #24

IvanScoles · 2018-07-19T05:43:09Z

Higher order components

Login form as HOC
Main form from LoginPage integrated with HOC from Login
Top form from NavBar integrated with HOC from Login
Navbar integrated with login page

Ready to review

Description

Here we have 2 forms with the same structure from form data username and password, to manage the same setState and use the same action from login (HOC). But we can see two ways to show the form information LoginFormMain and LoginFormTop both wrapper by the HOC

Removed lodash omit from components fixed a bug un userActions tests changed actions with more than 1 parameter to payload created utils functions to replace lodash utilites

javier-pepe · 2018-07-19T13:43:39Z

webapp/app/components/pages/login/LoginFormTop.jsx

+  static propTypes = {
+    username: PropTypes.string.isRequired,
+    password: PropTypes.string.isRequired,
+    handleOnChange: PropTypes.func.isRequired,


handleOnChange should not be required, rather a default prop value with a noop

static defaultProps = { handleOnChange: () => {} }

javier-pepe · 2018-07-19T13:44:08Z

webapp/app/components/pages/login/LoginFormTop.jsx

+    username: PropTypes.string.isRequired,
+    password: PropTypes.string.isRequired,
+    handleOnChange: PropTypes.func.isRequired,
+    handleOnSubmit: PropTypes.func.isRequired


handleOnSubmit should not be required, rather a default prop value with a noop

static defaultProps = { handleOnSubmit: () => {} }

javier-pepe · 2018-07-19T13:49:52Z

webapp/app/components/pages/login/LoginHOC.jsx

+const LoginHOC = (LoginChild) => {
+  class LoginParent extends React.Component {
+    static propTypes = {
+      actions: PropTypes.object.isRequired


we could use PropTypes.shape instead of PropTypes.object

actions: PropTypes.shape({ foo: PropTypes.func, bar: PropTypes.func })

actions should not be a required prop, maybe using defaults with no operations for expected actions

Good catch, good practice to do.

javier-pepe · 2018-07-19T13:52:38Z

webapp/app/components/pages/login/LoginHOC.jsx

+
+      const { username, password } = this.state;
+
+      this.props.actions.login({


if a variable has the same name as a key, it can be assigned in the following way instead

this.props.login({ username, password });

otherwise the state destructuring into { username, password } creating intermediate variables would be unnecessary since we could just do the following and avoid the intermediate variable creation. The shorthand version stated above should be preferred though.

({ username: this.state.username, password: this.state.password })

javier-pepe · 2018-07-19T14:04:26Z

webapp/app/components/pages/login/LoginHOC.jsx

+    handleOnChange = (e) => {
+      const { name, value } = e.target;
+
+      this.setState({


we should add at least a couple of basic checks, at least if name is not falsy, and if the value is different than the prior state value for that name key, only then updat ethe state for that key with setState,
otherwise we endup updating unecesarily a key with the same exact value in the state,
and in the unlickely case the name is undefined or empty, we endup invoking setStat e with invalid keys or causing exceptions.

Perhaps the use of something along these lines would be more efficient

if (name && (this.state[name] !== value || !this.state[name])) { this.setState({ [name]: value }); }

javier-pepe · 2018-07-19T14:33:23Z

webapp/app/components/pages/login/LoginHOC.jsx

+
+  const mapStateToProps = state => {
+    return {
+      auth: state.auth


we could rewrite this using destructuring and shorthand object assignment

const mapStateToProps = ({ auth }) => ({ auth });

javier-pepe · 2018-07-19T14:36:52Z

webapp/app/components/pages/login/LoginPage.css

@@ -0,0 +1,27 @@
+.card-container.card {
+    max-width: 350px;
+    padding: 40px 40px;


if the outer padding is the same value (vertical == horizontal),
there would be no need to specify them separately.

padding: [vertical] [horizontal];

rather please use

padding: 40px;

javier-pepe · 2018-07-19T14:41:39Z

webapp/app/components/pages/login/LoginPage.css

+    background-color: #F7F7F7;
+    padding: 20px 25px 30px;
+    margin: 0 auto 25px;
+    margin-top: 50px;


no need to override margin-top separately, it could be defined directly like so

margin: [top] [horizontal] [bottom];

Please replace the following

margin: 0 auto 25px; margin-top: 50px;

with the appropriate definition
margin: 50px auto 25px;

javier-pepe · 2018-07-19T14:53:40Z

webapp/app/components/pages/login/LoginPage.test.js

-
-      expect(keys(props.actions)).toEqual(expectedActions);
-    });
+    expect(wrapper.find('section')).toHaveLength(1);


Please add missing tests to make this one and all other new tests valid and more robust,

Testing we have a section element is valid, but alongside all other components and elements we are expecting to have in the rendered component.
We should test against functionality rather than implementation.

otherwise testing we just have a section expect(wrapper.find('section')).toHaveLength(1);
is fragile since the section element could be completely empty and the unit test will still pass. but in fact the application will be broken to the user.

The more robust way to test here would be to check against the components we are expecting and the actual output rendered.

There are at least 5 required checks missing here.

let section; beforeAll(() => { section = wrapper.find('section'); });

LoginNavigation

it('should have one <LoginNavigation>', () => { expect(section.find('LoginNavigation').length).toBe(1); });

Container

it('should have one `div` with class `container`', () => { expect(section.find('div.container').length).toBe(1); });

Card

it('should have one card `div` with class `card-container`', () => { expect(section.find('div.card.card-container').length).toBe(1); });

Profile Image

it('should have one `img` with class `profile-img`', () => { // optionaly we could also check against the image url expect(section.find('img.profile-img').length).toBe(1); });

LoginFormMain

it('should have one `<LoginFormMain>` ', () => { expect(section.find('LoginFormMain').length).toBe(1); });

I put a comment in the initial PR when was created:
Tests and clean code are pending

I'm working on this, base PR.

javier-pepe · 2018-07-19T15:14:29Z

webapp/app/components/pages/navbar/LoginNavigation.jsx

@@ -0,0 +1,64 @@
+import React from 'react';


I could not find the test for this component, does it have one?

javier-pepe · 2018-07-19T15:16:21Z

webapp/app/components/pages/login/LoginHOC.jsx

@@ -0,0 +1,72 @@
+import React from 'react';


Could not find the test for this component, does it have one?

javier-pepe · 2018-07-19T15:16:51Z

webapp/app/components/pages/login/LoginFormTop.jsx

@@ -0,0 +1,55 @@
+


Could not find corresponding test

javier-pepe · 2018-07-19T15:17:14Z

webapp/app/components/pages/login/LoginFormMain.jsx

@@ -0,0 +1,54 @@
+import React from 'react';


Could not find test

javier-pepe · 2018-07-19T15:21:07Z

webapp/app/components/common/modal/MsModal.jsx

+    } = this.props;
+    let dataToRender = null;
+
+    if (this.props.showFooter) {


Could not find the added test case for this newly added if (this.props.showFooter) branch, this will lower our branches coverage.

We should make sure to test all newly intoduced modifications and functionality, and as a rule of thumb always bring our coverage up or at least keep it the same after modifications or additions.

We should add the corresponding test, thanks

…from login page.

Emanuel Pereyra and others added 4 commits July 17, 2018 18:17

changes to component proptypes and class components

912e351

modified action types

fd0015f

More comments addressed

13ac37c

Removed lodash omit from components fixed a bug un userActions tests changed actions with more than 1 parameter to payload created utils functions to replace lodash utilites

HOC implementation with connect from redux and login form. POC

9e600f9

mravinale requested review from emanuelmetal, javier-pepe and mravinale July 19, 2018 13:26

mravinale assigned emanuelmetal Jul 19, 2018

javier-pepe suggested changes Jul 19, 2018

View reviewed changes

javier-pepe added the Feature A new feature/functionality/improvement label Jul 19, 2018

IvanScoles added 2 commits July 19, 2018 18:20

Adding coverage to new components from HOC. New navigation component …

1c86fa4

…from login page.

LoginHOC unit tests, code review changes.

79707e2

IvanScoles changed the base branch from 25-address-general-comments to develop July 23, 2018 00:50

IvanScoles added 3 commits July 22, 2018 22:22

Merge branch 'develop' into higher-order-components

a8b4f32

Adding more coverage to LoginFormMain and LoginFormTop components.

08ea0f8

Removed old login form.

dc7d153


		const { username, password } = this.state;

		this.props.actions.login({

HOC - Higher order components #24

Are you sure you want to change the base?

HOC - Higher order components #24

Uh oh!

Conversation

IvanScoles commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Higher order components

Ready to review

Description

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!

javier-pepe Jul 19, 2018 • 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!

javier-pepe Jul 19, 2018 • 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!

javier-pepe Jul 19, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

javier-pepe Jul 19, 2018 • 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!

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!

javier-pepe Jul 19, 2018 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

IvanScoles commented Jul 19, 2018 •

edited

Loading

javier-pepe Jul 19, 2018 •

edited

Loading

javier-pepe Jul 19, 2018 •

edited

Loading

javier-pepe Jul 19, 2018 •

edited

Loading

javier-pepe Jul 19, 2018 •

edited

Loading

javier-pepe Jul 19, 2018 •

edited

Loading