Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the prop types #1059

Merged
merged 11 commits into from Sep 17, 2019

Conversation

@bpierre
Copy link
Member

commented Sep 12, 2019

Builds on #1058

  • Templates kit
  • Components
  • Onboarding
bpierre added 5 commits Sep 12, 2019
- Remove longDesc and caseStudyUrl template entries (until we need
them).
- Move and rename the screen components into screens/.
- Add a prop type for screen-specific props.
- Unify the way screen components accept screen-specific props.
- Rename PrevNextFooter => Navigation.
@bpierre bpierre requested review from AquiGorka and sohkai Sep 12, 2019
@bpierre bpierre marked this pull request as ready for review Sep 12, 2019
Also renamed userGuide to userGuideUrl in the template format.
@bpierre

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

@sohkai @AquiGorka PR ready! 🚦

@bpierre bpierre referenced this pull request Sep 13, 2019
3 of 17 tasks complete
Copy link
Contributor

left a comment

👌 👌

@now now bot temporarily deployed to staging Sep 17, 2019 Inactive
@now now bot temporarily deployed to staging Sep 17, 2019 Inactive
@bpierre bpierre merged commit 32e47b9 into master Sep 17, 2019
3 of 7 checks passed
3 of 7 checks passed
install install
Details
License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
now Now is deploying your app
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@delete-merged-branch delete-merged-branch bot deleted the fix-prop-types branch Sep 17, 2019
@@ -167,6 +168,12 @@ function Tokens({
}
}, [])

// Focus the token fields as they are added afterwards

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

Was this intended to be commented out?

This comment has been minimized.

Copy link
@bpierre

bpierre Oct 9, 2019

Author Member

No that should be removed!

screenSubtitle = 'Have one last look at your settings below',
screenTitle = 'Review information',

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

I think we can remove these defaults, since they're supplied by the default props now

screenTitle = 'Claim a name',
},
screenProps: { back, data, next, screenIndex, screens },
screenTitle = 'Claim a name',

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

Same as ReviewScreen, I think we can remove the defaults for dataKey and screenTitle :)

screenIndex: PropTypes.number.isRequired,

/*
* No tuples in prop-types… if it existed, screens would look like this:

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

We could technically define our own validation for a tuple

@@ -0,0 +1,23 @@
import PropTypes from 'prop-types'

export const ScreenPropsType = PropTypes.shape({

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

In prop-types we've used the Type suffix only, rather than PropsType. What do you think about changing this to just ScreenType?

This comment has been minimized.

Copy link
@bpierre

bpierre Oct 9, 2019

Author Member

It’s the Type suffix added to screenProps. I know it’s confusing 😆

@@ -140,4 +140,8 @@ function CreateSubtitle({ error }) {
return 'Start your organization with Aragon'
}

CreateSubtitle.propTypes = {
error: PropTypes.array.isRequired,

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

Looking back at this, we should probably use an object with named fields 😄

@@ -51,10 +56,10 @@ function Configure({
overflow: hidden;
`}
>
{mode === 'select' && (
{mode === CONFIGURE_MODE_SELECT && (

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

I'm starting to think, maybe we should take the template selection part out of configure?

It'd probably simplify a lot of the props we pass around if we split these two (selecting template vs. configuring selected), and I don't think we have any transitions that depend on these two being together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.