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

[Claim] Context state #2064

Merged
merged 9 commits into from
Jan 11, 2022
Merged

[Claim] Context state #2064

merged 9 commits into from
Jan 11, 2022

Conversation

W3stside
Copy link
Contributor

Summary

As discussed, this is the context state implementation for claiming. It creates a similar actions/reducer file structure in the Claim folder and allows passing of state without "props drilling"

As a working example, I extrapolated "EligibleBanner" into Claim > EligibleBanner.tsx and it is used in index.tsx

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@@ -0,0 +1,21 @@
import { Trans } from '@lingui/macro'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is an example (to be kept) to show how it will work. Next PR #2072 goes hard on this

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.

And here I thought we would have less boilerplate

🤷

Comment on lines 1 to 19
export const enum ActionTypes {
// accounts/address
setInputAddress = 'setInputAddress',
setActiveClaimAccount = 'setActiveClaimAccount',
setActiveClaimAccountENS = 'setActiveClaimAccountENS',
// claiming
setClaimConfirmed = 'setClaimConfirmed',
setClaimAttempting = 'setClaimAttempting',
setClaimSubmitted = 'setClaimSubmitted',
setClaimedAmount = 'setClaimedAmount',
// investment
setIsInvestFlowActive = 'setIsInvestFlowActive',
setInvestFlowStep = 'setInvestFlowStep',
// search
setIsSearchUsed = 'setIsSearchUsed',
// selected claim rows (table/UI)
setSelected = 'setSelected',
setSelectedAll = 'setSelectedAll',
}
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 Author

Choose a reason for hiding this comment

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

in fairness tho, it's TS that adds a lot of bloat, tho it's helpful innit. Maybe i should consider using the createActions here actually 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please go for Recoil in the next project 🥺

@W3stside
Copy link
Contributor Author

And here I thought we would have less boilerplate

🤷

hahah yeah i made a comment about this in the morning sync. figured its better here tho rather than cluttering the global object.

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.

Thanks for making the effort to refactor. I can tell you had to add some thought into it.
However... Are we sure we want to go this path? I'm am very hesitant

I understood you would add Context, which I was also not super sure it would be perfet for this because I was not expecting a deep hierarchy.
You not only added context, but also are using a reducer.

One other concern with this PR, is how we have modeled the state and actions and the state. It seems there's way more stuff than necessary.

These 3 things (context, actions with lots of boilerplate code, not so good modeling) makes this kind of hard to mantains in my eyes.

I thought we were going through the simple route, this is why we didn't add redux as the rest of the app has, but this approach seems even harder.

@anxolin
Copy link
Contributor

anxolin commented Jan 11, 2022

We discussed internally, and we will move this forwards, we cannot afford to loose too much time in the refactoring. However, given this PR already addes reducers, we also talk we will use redux as all the other parts of the app do

@anxolin anxolin merged commit 48d1b7a into claim Jan 11, 2022
@alfetopito alfetopito deleted the claim-context-state branch January 11, 2022 18:23
nenadV91 pushed a commit that referenced this pull request Jan 12, 2022
…en (#2064)

* initial off screen indicator

* adjust offscreen indicator

* add off screen handle indicator

* hide reset until we get a better behavior

* add svg.tsx
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