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

Templates kit cleanup #1057

Merged
merged 2 commits into from Sep 17, 2019

Conversation

@bpierre
Copy link
Member

commented 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.
- 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.
}) {
const [domain, setDomain] = useState(data.domain || '')
const screenData = (dataKey ? data[dataKey] : data) || {}

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 17, 2019

Contributor

No need to change but you could use data = {} as default above and there wouldn't be a need for the || {}

This comment has been minimized.

Copy link
@bpierre

bpierre Sep 17, 2019

Author Member

It also covers the case of having an undefined data[dataKey].

if (containerRef.current) {
containerRef.current.focus()
}
}, [])

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 17, 2019

Contributor

Should container.Ref be a dependency here?

This comment has been minimized.

Copy link
@bpierre

bpierre Sep 17, 2019

Author Member

ref containers are always the same, so I don’t think it is ever needed?

<div
ref={containerRef}
tabIndex="0"
css={`

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 17, 2019

Contributor

css="outline: 0;"

}) {
const screenData = (dataKey ? data[dataKey] : data) || {}

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 17, 2019

Contributor

No need for this no? dataKey has a default value

This comment has been minimized.

Copy link
@bpierre

bpierre Sep 17, 2019

Author Member

That’s because dataKey can be set to null explicitly, in which case the keys will be added do data directly.

],
...tokensData.members.map(([account], i) => [
...screenData.members.map(([account], i) => [
`Tokenholder #${i + 1}`,

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 17, 2019

Contributor

Should this be 2 words? Token holder #...?

This comment has been minimized.

Copy link
@bpierre

bpierre Sep 17, 2019

Author Member

No it was decided to use the term “Tokenholder” rather than “Token holder” from now on.

@dizzypaty @owisixseven @john-light @luisivan I’m not sure we have these rules written anywhere − should we start a document somewhere for content guidelines?

dataKey = 'voting',
screenProps: { back, data, next, screenIndex, screens },
}) {
const screenData = (dataKey ? data[dataKey] : data) || {}

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 17, 2019

Contributor

Same as above, with the default value there is no need to check here.

Copy link
Contributor

left a comment

Some comments, nothing big, 👍

@bpierre bpierre merged commit 7614ac1 into master Sep 17, 2019
5 of 9 checks passed
5 of 9 checks passed
build build
Details
install install
Details
lint lint
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 templates-api-cleanup branch Sep 17, 2019
Copy link
Member

left a comment

@bpierre Just noticing that ClaimDomainScreen and the Voting and Token configuration screens handle dataKey differently; was this intended?

For the two apps, it seems that if you set dataKey: null, the data won't be written whereas ClaimDomainScreen will default to 'domain'.

@bpierre

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

@sohkai Yes, I think we discussed it? This is because the domain is usually unique, and it avoids having { domain: { domain: 'something.aragonid.eth' } by default.

For the two apps, it seems that if you set dataKey: null, the data won't be written whereas ClaimDomainScreen will default to 'domain'.

If you set dataKey = null, the data will be written, but directly on the data object rather than nested using the key.

@sohkai

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

If you set dataKey = null, the data will be written, but directly on the data object rather than nested using the key.

Ahh yes, right, this was the intended behaviour.

I think I might've found a different issue though, since we were working on the onboarding so quickly before. I know we were discussing merging the state when calling next() in a template screen, but I don't think that's happening (note the lack of a merge here: https://github.com/aragon/aragon/blob/master/src/onboarding/Create/Create.js#L143). If we provide a null dataKey, then we've overwritten any old template state with either the VotingScreen or TokenScreen's (e.g. see how TokenScreen just passes its own data to next()).

I forget which direction we ended deciding for the merge in next(), but I'm seeing that we probably still want a merge to happen, and it would also simplify this operation to:

next(dataKey ? { dataKey]: screenData } : screenData)
@sohkai

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

If we provide a null dataKey, then we've overwritten any old template state with either the VotingScreen or TokenScreen's (e.g. see how TokenScreen just passes its own data to next()).

I see this was fixed in #1091 :). Sorry for all the noise!

@bpierre

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

Oh right! Perfect 👍

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.