Skip to content
This repository has been archived by the owner on Aug 5, 2022. It is now read-only.

Week 7 Demo #15

Merged
merged 6 commits into from
May 3, 2019
Merged

Week 7 Demo #15

merged 6 commits into from
May 3, 2019

Conversation

koss-lebedev
Copy link
Contributor

No description provided.

Copy link
Contributor

@prichodko prichodko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 👍

I've left just a single comment.

When it comes to typing Redux, I always feel like balancing on a line what should be typed and what may not. But I think you did the right amount. You may also explore if some helper types like ThunkAction or Reducer would simplify the code a bit more, but it's not a big deal anyway.

@@ -1,11 +1,16 @@
import React, { Fragment } from 'react'
import React, { Fragment, SFC } from 'react'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the arrival of the hooks SFC became obsolete, because functional components are not necessarily stateless anymore. Please use the new FunctionalComponent or FC type. 👍

You can read more here: DefinitelyTyped/DefinitelyTyped#30364

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I knew about this stuff but completely forgot which one is deprecated 😄

import * as routes from '../../routes'

import { Wrapper, Header, HeaderSection, HeaderLink } from './styled'

const Layout = ({ isAuthenticated, children, dataTestId }) => (
type Props = ReturnType<typeof mapStateToProps> & {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@dannytce dannytce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

I ran into several issues though running yarn lint:js

  1. Unable to resolve path to module '../../components/Layout' import/no-unresolved
    This might be fixed by adding eslint-import-resolver-typescript, or maybe there is a better way, idk
  2. There is an error with import/named rule, when exporting types (for example ProductType). Could be fixed with installing eslint-plugin-import (version 2.17 and higher). But to be honest this probably should be fixed on code-quality-tools instead. But not sure when this is going to happen.

package.json Outdated
"eslint --fix",
"prettier --write",
"stylelint",
"tsc --skipLibCheck",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe yarn type-check instead? So there both scripts are in sync?

@dannytce dannytce merged commit a6968d1 into master May 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants