-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Manage User Badges in the UI #31262
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
Manage User Badges in the UI #31262
Conversation
9983632 to
91b085b
Compare
91b085b to
9bd77fd
Compare
9bd77fd to
5cc1147
Compare
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.
Thanks for the PR! The CI has a few linting reports that came up that I've reported in line :)
5cc1147 to
f378b9b
Compare
Thanks for the PR! The CI has a few linting reports that came up that I've reported in line :)
Thank you so much!
3f64e40 to
5cbed8a
Compare
Co-authored-by: Diogo Vicente <diogo.m.s.vicente@tecnico.ulisboa.pt>
Co-authored-by: Diogo Vicente <diogo.m.s.vicente@tecnico.ulisboa.pt>
5cbed8a to
d681313
Compare
@HenriquerPimentel looks like some different CI steps are failing, also related to linting (some public go functions require comments to document their purpose, eg AdminCreateBadge), although with AdminCreateBadge as it is a wrapper for another function without any transformative properties or checks that one could probably be kept as just CreateBadge without the secondary function.
As a sidenote: force pushing erases some review history as GitHub sees some files as new (even if they remain the same). If you could reduce your force pushes, it'd be helpful to me as a reviewer so I can keep track of what I've seen already
disclaimer: most of the time we try to discourage force pushing to PRs for this reason, although depending on the reviewer of a certain PR they can "call an audible" depending on the situation and might be ok with it.
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.
Two bugs:
- string variables used in a URL should be escaped (and other places)
- this
ifblock will continue executing, then you have 2 JSONRedirect calls in one response.
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.
@go-gitea/maintainers
I have suggested many times to have a better framework to handle various errors and returns.
You can see that how many bugs caused by our stupid framework in this PR.
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.
Why single SQL needs transaction?
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 is already a transaction in DeleteBadge.
So why we need this wrap function in service package?
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.
Some questions:
- Why parent "div" has "disabled=disabled" and inner "input" also has "disabled=disabled"?
- If there is no input (no change), why
{{if .Err_Slug}}would occurs? - If the input is disabled, why the "label" has "for"?
- What does "non-local" mean here?
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.
Our frontend framework can handle most a11y cases, so no need to repeat id and for (and other places)
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.
delete class is a no-op IIRC
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.
Why the search form is at the bottom?
IIRC for all (at least almost) cases Gitea layout is like this: search form at top, table at bottom.
ps: why it needs an id? I don't see it is used anywhere.
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.
Hmm ... it is not a real "search" form, but it is used to add a new user, so maybe it's fine.
But still have the question why there are unused id attributes.
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.
Many "div" open/close tags misaligned, I think it's better to make the tags align correctly to make code readable.
Implemented #29798
This feature implements:
Implemented Badge Management in administration panel
9 Added new translation phrases (en-US): search.badge_kind, form.ImageURL, form.invalid_image_url_error, form.slug_been_taken, admin.badges, admin.badges.badges_manage_panel, admin.badges.details, admin.badges.new_badge, admin.badges.slug, admin.badges.description, admin.badges.image_url, admin.badges.slug.must_fill, admin.badges.new_success, admin.badges.update_success, admin.badges.deletion_success, admin.badges.edit_badge, admin.badges.update_badge, admin.badges.delete_badge, admin.badges.delete_badge_desc
Implemented User Badge Management Interface