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

[Claim refactor index] - Part 1 - Extrapolate 5 small components (summary, nav, claim messages) #2072

Merged
merged 7 commits into from
Jan 11, 2022

Conversation

W3stside
Copy link
Contributor

Summary

Extrapolates some claim > index.ts code into 5 small components (summary, nav, claim messages)
Uses the claim context to make this easier

Can be tested by just using the claim flow. same info should show as claim build

@W3stside W3stside requested review from anxolin and a team January 10, 2022 18:52
@W3stside W3stside changed the base branch from develop to claim-context-state January 10, 2022 18:52
@W3stside W3stside self-assigned this Jan 10, 2022
@W3stside W3stside mentioned this pull request Jan 10, 2022
Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Code looks good, although PR deployment didn't trigger so I did not test the flow

@W3stside W3stside force-pushed the claim-refactor-index branch from ebc4872 to 91e7773 Compare January 10, 2022 19:39
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@alfetopito
Copy link
Contributor

Tried it out now, flow looks the same 👍

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Nice you broke it down, thanks for refactoring, however not so fan of the current approach.

Anyways, we will move forwards and try to make this look more similar to other components of the app (use redux state) and simplify the state (i.e. don't add so many booleans for mutually exclusive state

import { IntroDescription } from './styled'
import { ClaimCommonTypes } from './types'

type ClaimIntroductionProps = Pick<ClaimCommonTypes, 'hasClaims'> & {
Copy link
Contributor

Choose a reason for hiding this comment

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

is all this better than adding just a hasClaims: boolean

if we use the approach i sugges:
CONS:

  • yes if hasClaims change type, it will be automatic

PRO

  • Way easier to read
  • More self contain component. You don't need to artificially get some other type to get a field from it

@anxolin anxolin force-pushed the claim-refactor-index branch from 8ed0258 to c99d373 Compare January 11, 2022 17:28
@anxolin anxolin changed the base branch from claim-context-state to claim January 11, 2022 17:28
@anxolin anxolin merged commit f92674b into claim Jan 11, 2022
@alfetopito alfetopito deleted the claim-refactor-index branch January 11, 2022 18:27
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.

3 participants