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

feat(govdao): better rendering #3096

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

leohhhn
Copy link
Contributor

@leohhhn leohhhn commented Nov 8, 2024

Description

Introduces better gnoweb rendering for the GovDAO suite, and better help page actions for voting on proposals.

Home page before:
Screenshot 2024-11-08 at 16 29 49

Home page after (also resolves usernames from r/demo/users):
Screenshot 2024-11-09 at 13 19 55

Prop page before:
Screenshot 2024-11-08 at 16 30 43

Prop page after:

Screenshot 2024-11-21 at 13 05 50

The actions bar notifies the user when the proposal is no longer active as well.

Continuation of #2579

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests

@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Nov 8, 2024
Copy link

codecov bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@Kouteki Kouteki changed the title feat(govdao): better rednering feat(govdao): better rendering Nov 8, 2024
@moul
Copy link
Member

moul commented Nov 8, 2024

@leohhhn can you add the prop title in the list too? Maybe just the N first chars.

@leohhhn
Copy link
Contributor Author

leohhhn commented Nov 8, 2024

@moul

I was about to; but then i realized there isn't a title field in the prop structure/interface.

Is there a reason for this? If not, I can add it (I did the same thing in my previous PR)

@moul
Copy link
Member

moul commented Nov 8, 2024

Yeah, I think it makes sense for a prop to have a required title and an optional or required description.

@moul moul mentioned this pull request Nov 9, 2024
@leohhhn leohhhn requested a review from moul November 9, 2024 12:23
examples/gno.land/p/demo/simpledao/propstore.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/simpledao/propstore.gno Outdated Show resolved Hide resolved
examples/gno.land/r/gov/dao/v2/dao.gno Outdated Show resolved Hide resolved
@@ -58,5 +61,5 @@ type Proposal interface {
IsExpired() bool

// Render renders the proposal in a readable format
Render() string
Render(idx int, authorUsername string) string
Copy link
Member

Choose a reason for hiding this comment

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

The caller shouldn't need to specify these things, the Proposal should know about them internally

Copy link
Member

Choose a reason for hiding this comment

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

It can be a phishing vector

Copy link
Contributor Author

@leohhhn leohhhn Nov 12, 2024

Choose a reason for hiding this comment

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

Two issues:

  • The Proposal interface doesn't have a built-in ID - should we add this? Otherwise, prop page rendering can be done inside of the r/gov/v2 realm, but it will be split up between r/gov/v2 & simpledao, making it a bit ugly.
  • This is a p/ - to resolve usernames, we need to access r/demo/users; importing a r/ inside of a p/ will be forbidden soon. We can set the username upon prop creation, but it will not be updated later if a user is not registered initially and then registers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this; check it out:

05253fc & 6ed08b0

out.WriteString(ufmt.Sprintf("#### Status: %s\n\n", strings.ToUpper(p.Status().String())))

out.WriteString(ufmt.Sprintf(
"#### Voting stats:\n- YES %d (%d%%)\n- NO %d (%d%%)\n- ABSTAIN %d (%d%%)\n- MISSING VOTE %d (%d%%)\n",
Copy link
Member

Choose a reason for hiding this comment

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

MISSING VOTES?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used to be like that, I assumed there was a reason; changed it: 1a9c7ed

moul added a commit that referenced this pull request Nov 15, 2024
Can be useful for #3096.

---------

Signed-off-by: moul <[email protected]>
@Kouteki Kouteki mentioned this pull request Nov 20, 2024
@leohhhn
Copy link
Contributor Author

leohhhn commented Nov 21, 2024

@moul @ltzmaxwell

Made some more changes. Check out the comments & the prop page that was updated a bit.

@gh-gno-bot
Copy link

gh-gno-bot commented Nov 29, 2024

Merge Requirements

The following requirements must be fulfilled before a pull request can be merged.
Some requirement checks are automated and can be verified by the CI, while others need manual verification by a staff member.

These requirements are defined in this configuration file.

Automated Checks

🟢 Maintainers must be able to edit this pull request
🟢 The pull request head branch must be up-to-date with its base

Details
Maintainers must be able to edit this pull request

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Head branch (leohhhn:govdao2-rendering) is up to date with base (master): behind by 0 / ahead by 36

Manual Checks

  • The pull request description provides enough details
Details
The pull request description provides enough details

If

🟢 Condition met
└── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)

Can be checked by

  • team core-contributors

@leohhhn leohhhn enabled auto-merge (squash) November 30, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

6 participants