-
Notifications
You must be signed in to change notification settings - Fork 391
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(examples/hof): Title and description for realms, sorting by upvotes and newest #3674
base: master
Are you sure you want to change the base?
Conversation
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
examples/gno.land/r/leon/hof/hof.gno
Outdated
func RegisterMetadata(params ...string) Metadata { | ||
// Validate params length | ||
if len(params) != 2 { | ||
panic(ErrInvalidMetadata) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bad approach, you're using variadic arguments but you know that you will have only two arguments.
Variadic arguments should be used only in specific cases, and this isn't one of them, as you could've done this in a very straighforward manner with two arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed variadic arguments and used 2 arguments instead: 02edc70
examples/gno.land/r/leon/hof/hof.gno
Outdated
exhibition.items.Set(pkgpath, i) | ||
exhibition.itemsSorted.Set(id.String(), i) | ||
|
||
std.Emit("Registration") | ||
} | ||
|
||
func RegisterMetadata(params ...string) Metadata { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this whole function; Just add the title and description in the Register function directly. Keep the checks, but don't panic but just add an empty return, just like in the rest of the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine how your code will be used; if you need to use Register inside of another realm's code, if you panic, you will fully block execution of the realm that called Register. If you simply do a return, without an error, in Register, HOF will try to add the realm that called it, it may work, but if not, nothing will happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed RegisterMEtadata functon, also replaced panic with return in this commit: 02edc70
examples/gno.land/r/leon/hof/hof.gno
Outdated
upvote *avl.Tree // std.Addr > struct{}{} | ||
downvote *avl.Tree // std.Addr > struct{}{} | ||
} | ||
|
||
Metadata struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a reason to have this, just add two strings to the Item
type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit: 02edc70
@@ -45,7 +46,7 @@ func (e Exhibition) Render(path string, dashboard bool) string { | |||
|
|||
out += "<div>\n\n" | |||
id, _ := seqid.FromString(item.Key) | |||
out += ufmt.Sprintf("### Submission #%d\n\n", int(id)) | |||
out += md.H3(ufmt.Sprintf("Submission #%d", int(id))) + "\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a \n
needed here? I'm not sure, please double check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this in last commit, markdown is a lot different from this: 02edc70
out := "" | ||
|
||
out += md.CodeBlock(i.pkgpath) + "\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a screenshot of how the new hall of fame page will look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to change the approach in your implementation; I've left comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of things, some of which are the fundamentals of collaborating on GitHub:
- Please, when someone leaves comments, reply to them properly; ideally you also add the commit in which you fixed what the comment was about.
- When you add new functionality somewhere, you also need to write tests to cover that. I tried using the app and clicking the "sort by upvotes", but it just doesn't work.
- You need to update the PR title and description to match what's going on in the PR, which isn't the case right now.
- When you finish your work on a PR, you need to check the CI, which will tell you if something is wrong (ie the tests fail currently)
Btw; you removed the sorting functionality that the app originally had (sort by latest) and replaced it with the "sort by upvotes". The default sort should be by time of addition, while the other option should be sort by upvotes (most and least). Also, you've changed the datasource impl, and I'm assuming that you didn't check out where it's used - I suggest you keep it the same as before, as someone might be depending on it.
You also made some other mistakes, but we'll get to them once the above is resolved.
This is all a learning process; it's ok that you make mistakes, but I would like to see you put in a bit more time, effort and focus into your work, and not do things last minute & rushed.
b55f906
to
262e8ff
Compare
…red on HoF in order to match new Register function signature
…ber and description
…ts for datasource to match new fields, added tests in hof_test to test new cases
…ting sorting filter
…n for generating keys, other minor improvements
description: "This is a test realm in the Hall of Fame", | ||
blockNum: 42, | ||
upvote: avl.NewTree(), | ||
downvote: avl.NewTree(), | ||
} | ||
item.downvote.Set("g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5", struct{}{}) | ||
item.upvote.Set("g1w4ek2u33ta047h6lta047h6lta047h6ldvdwpn", struct{}{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw @leohhhn; you can consider using p/moul/addrset to make this more efficiently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ursulovic Let's switch to p/moul/addrset
for managing the number of upvotes/downvotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leohhhn Will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in commit: c576d01
Title And Description for Hall of Fame realms
Goal
Add title and description to realms in the Hall of Fame, as well as functionality of sorting realms by upvotes and by date of publishing.
Improvements
Title and Description
Realms in Hall of Fame will now have title and description, this will give users more context about each realm.
From now on, people who wish to publish their realm in the Hall of Fame will have to provide a title and description when registering their realm.
Sorting Realms by Upvotes and Newest realms
Realms in the Hall of Fame will now have the functionality to be sorted by upvotes and most recent realms. This will improve user experience by making it easier to find specific realms or explore new ones.
Feature Progress 🔄