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

[Governance] proposal metadata from url #47

Merged
merged 3 commits into from
Jul 25, 2022
Merged

[Governance] proposal metadata from url #47

merged 3 commits into from
Jul 25, 2022

Conversation

0xZihan
Copy link
Contributor

@0xZihan 0xZihan commented Jul 21, 2022

To align with change in aptos-labs/aptos-core#2121, this PR display proposal content metadata (description, code) from and external url.

Next step:

Screen Shot 2022-07-21 at 3 14 57 PM

@@ -21,6 +21,25 @@ export function ProposalContent({
return null;
}

const [metadata, setMetadata] = useState<any>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add a type instead of any (e.g. it appears to be an object that has description and execution_script properties)?

};

fetchMetadata();
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it re-fetch the metadata if metadata_location changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment! metadata_location comes from the chain so it's probably won't be changed. Although I'll apply the change to be safe.

const CODE_LOCATION_PLACEHOLDER = "https://aptoslabs.com/";
const TITLE_PLACEHOLDER = "Title -- Lorem ipsum dolor sit amet";
const METADATA_LOCATION_PLACEHOLDER =
"https://mocki.io/v1/32403c87-d5d2-4136-80fd-d639a5d3d7dd";
Copy link
Contributor

Choose a reason for hiding this comment

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

are we also getting the title from the metadata link? https://mocki.io/v1/32403c87-d5d2-4136-80fd-d639a5d3d7dd has title also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the PR summary, I'll add the title later to avoid merge conflicts.

Copy link
Contributor

@0xmaayan 0xmaayan left a comment

Choose a reason for hiding this comment

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

lgtm

@0xZihan 0xZihan merged commit 0452cd0 into main Jul 25, 2022
@0xZihan 0xZihan deleted the proposal-content branch July 25, 2022 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants