Batch delete issue and improve tippy opts #25253
Conversation
wxiaoguang
commented
Jun 14, 2023
•
wxiaoguang
commented
- Add "batch delete" button for selected issues, close Delete multiple issues at once #22273
- Address the review in Change form actions to fetch for submit review box #25219 (comment)
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/L
wxiaoguang
force-pushed
the
batch-delete-issue
branch
from
38ec3c3
to
cb30446
Compare
| delete opts.onDestroy; | ||
| delete opts.onShow; | ||
|
|
||
| const {onHide, onShow, onDestroy, ...other} = opts; |
There was a problem hiding this comment.
Would also destructure role and content here.
There was a problem hiding this comment.
Can even shorten it by placing the destructure in the function definition:
export function createTippy(target, {onHide, onShow, onDestroy, role, content, ...other} = {})There was a problem hiding this comment.
Would also destructure
roleandcontenthere.
IMO the code would be more complex for using "role" later. Could you try to improve it by your idea?
There was a problem hiding this comment.
export function createTippy(target, {onHide, onShow, onDestroy, role, content, ...other} = {})
Well, personally I dislike such syntax. But if you prefer, no block from my side.
There was a problem hiding this comment.
Definitely prefer that, it's most readable to me. Don't have time right now, but I can do a few modifications later.
options/locale/locale_en-US.ini
Outdated
| @@ -1412,6 +1412,7 @@ issues.filter_sort.fewestforks = Fewest forks | |||
| issues.keyword_search_unavailable = Currently searching by keyword is not available. Please contact your site administrator. | |||
| issues.action_open = Open | |||
| issues.action_close = Close | |||
| issues.action_delete_confirm = Confirm to delete all issues? | |||
There was a problem hiding this comment.
Would prefer something more generic:
delete_all_confirm = Delete all items?And likely move it to top-level translation.
There was a problem hiding this comment.
Done in 8960441
But I still do not think it is a generic string.
And the "generic" problem is: without careful maintenance, it will become more and more messy. For example, why rerun_all = Re-run all jobsappears in the global scope? It isn't generic indeed.
There was a problem hiding this comment.
If a translation could reasonably appear in more than 1 scope, it should be global, but I guess the one you posted is likely to be single-use.
Overall, I think we should just get rid of scopes altogether in the translation, removing all duplicates in the process. Those scope have no benefit except encourage duplication.
silverwind
changed the title
web_src/js/modules/tippy.js
Outdated
| delete opts.onDestroy; | ||
| delete opts.onShow; | ||
|
|
||
| export function createTippy(target, {onHide, onShow, onDestroy, role, content, ...other} = {}) { |
There was a problem hiding this comment.
Well, this is wrong now (that's what I said why it is really unclear).
Because the role is exacted, other doesn't contain role. Then how do you use ...other to override the default role ....
I reverted the code, the old code looks good to me.
There was a problem hiding this comment.
The only change that was needed would have been role: role || 'menu'. I still want to do my method.
There was a problem hiding this comment.
I think current style is the best, the reasons are:
export function createTippy(target, opts = {})is shorter and clearer for readers, it's easy to catch the function declaration.- I remember that you mentioned that you don't want to read a line with more than 100 chars, using destructing in the function declaration makes the line longer and longer.
- If you destructure "role", this time you can patch the code by
role: role || 'menu', next time if another variable is destructed, developers would forget the...otheroverriding again. It is quite fragile and unclear. For example, I have mentioned thatthe code would be more complex for using "role" later., the changed code still contains bugs, so it is a complex and counterintuitive problem.
There was a problem hiding this comment.
Well I guess we can keep it.
This reverts commit 868d059.
# Conflicts: # modules/context/base.go # web_src/js/features/common-global.js
GiteaBot
added
lgtm/need 1
lunny
added
the
type/feature
GiteaBot
added
lgtm/done
lunny
added
the
reviewed/wait-merge
GiteaBot
removed
the
reviewed/wait-merge
* giteaofficial/main: Fix incorrect actions ref_name (go-gitea#25358) Make backend code respond correct JSON when creating PR (go-gitea#25353) Fix loading state regression in markup content (go-gitea#25349) Batch delete issue and improve tippy opts (go-gitea#25253)
