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

Preview freeze pallet proposal #4627

Merged
merged 9 commits into from
Nov 14, 2023

Conversation

vrrayz
Copy link
Contributor

@vrrayz vrrayz commented Nov 10, 2023

Preview freeze pallet proposal and continuation of this PR #4625

┆Issue is synchronized with this Asana task by Unito

Copy link

vercel bot commented Nov 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
dao ✅ Ready (Inspect) Visit Preview Nov 14, 2023 3:41pm
pioneer-2 ✅ Ready (Inspect) Visit Preview Nov 14, 2023 3:41pm
pioneer-2-storybook ✅ Ready (Inspect) Visit Preview Nov 14, 2023 3:41pm

onChange: (option: PalletFrozenStatus) => void
}

export const OptionsPalletFrozenStatus = React.memo(({ onChange }: Props) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think this file is not needed

selectedStatus?: boolean
onChange: (selected: PalletFrozenStatus) => void
}
export const SelectPalletFrozenStatus = ({ selectedStatus, onChange }: Props) => {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

export const UpdatePalletFrozenStatus = () => {
const { watch, setValue } = useFormContext()

const setPalletFrozenStatus = (selected: PalletFrozenStatus) => {
Copy link
Member

Choose a reason for hiding this comment

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

And this one too

@@ -80,6 +80,10 @@ export type UpdateChannelPayoutsDetail = {
payloadHash?: string
payloadDataObjectId?: string
}
export type UpdatePalletFrozenStatusDetail = {
frozen?: boolean
Copy link
Member

Choose a reason for hiding this comment

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

Here too please change frozen to freeze because it's the action that the proposal should apply (not the current state).

{
label: 'Proposed Status',
renderType: 'Text',
value: value ? 'Enable' : 'Disable',
Copy link
Member

Choose a reason for hiding this comment

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

I think it's the opposite if we want to freeze the pallet the proposed status is "Disable" otherwise "Enabled".

Also please rename value to freezePallet

renderType: 'Text',
value: value ? 'Enable' : 'Disable',
},
]
Copy link
Member

Choose a reason for hiding this comment

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

Also the pallet should be displayed too

value: value ? 'Enable' : 'Disable',
},
]
}
Copy link
Member

Choose a reason for hiding this comment

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

And I'm wondering if the current pallet state should be displayed too. WDYT @dmtrjsg @chrlschwb ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think that's good to have.

@@ -359,6 +369,12 @@ const asUpdateChannelPayouts: DetailsCast<'UpdateChannelPayoutsProposalDetails'>
payloadDataObjectId: extra?.payloadDataObjectId,
})

const asUpdatePalletFrozenStatus: DetailsCast<'UpdatePalletFrozenStatusProposalDetails'> = (fragment) => ({
type: 'updatePalletFrozenStatus',
frozen: fragment.frozen,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
frozen: fragment.frozen,
freeze: fragment.frozen,

Copy link
Member

@thesan thesan left a comment

Choose a reason for hiding this comment

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

Nice work 🙌

@thesan thesan merged commit 4af43dc into Joystream:nara Nov 14, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-dev issue suitable for community-dev pipeline jsg-code-review nara-network Nara Network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants