-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Show depending and blocked PRs in the PR list #29117
base: main
Are you sure you want to change the base?
Show depending and blocked PRs in the PR list #29117
Conversation
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 things I noticed while testing:
- We should swap the order of
blocks
/depends on
around, as thedepends on
is much more important in most cases, so should be the first info you see. - There's no clear separator when both are present, making the display slightly confusing. Not a blocker.
Apart from that, there might be some performance problems when querying a repo with many PRs/ a large dependency network. However, I don't know if they are (easily) fixable.
Otherwise LGTM.
I would lowercase the text. All other text on the line is lowercase, so it looks out of place to title-case. |
This is my subjective opinion as someone who made the feature request, items in priority order (this PR might already do some of them in that case ignore those points):
With the exception of the first point if the things above sound unreasonable or they might not align with the majority of the people I'm ok with the current state of this PR. |
|
UI looks good. Only thing that maybe could be improved is to manually emit the separators in HTML instead of CSS because I fear that the CSS selector may have false matches, but at least from the screenshots, it looks alright. |
Ah, it works now after enabling that. I hadn't expected it to be locked behind a setting. |
Its because of #29117 (comment) |
I think the rendering could be made similar to milestone, with an icon before an no text besides issue numbers joined by ", ". The question is just which icons to use for "depends on" and "blocks". I don't think octicons has anything suitable, but https://www.svgrepo.com/ might. I noticed the dot we added causes unsatisfactory rendering for link hover. We need to fix that, maybe just remove the dot after all. A bit of whitespace should be enough between items. |
These two are pretty good already, We could switch the box to a rounded outlined square: https://primer.github.io/octicons/package-dependencies-16 There is also a fitting icon for a "issue blocks": https://primer.github.io/octicons/issue-tracked-by-16 The idea is that if the arrow in the icon indicates the direction of the dependency, pointing from icon to the numbers for "depends on" and from the numbers to icon for "blocks". |
@lunny I need the join repo for the link Line 401 in 9bf693d
gitea/web_src/js/features/contextpopup.js Line 17 in 9bf693d
|
Or even more simplified icons: https://primer.github.io/octicons/arrow-right-16 for "depends on" Each with a tooltip over the icon. If you agree I can do the necessary changes directly on the branch. |
I think this Icons are better |
One last thing, is it possible to change the color of the PR icon based on if the PR has unresolved dependencies? Creating a brand new icon for it is probably a stretch, even if Gitea currently does not use the PR closed icon for closed PRs. To indicate more clearly that the PR cannot be merged so it's not worth reviewing (or it's dependencies are worth reviewing instead). If the dependency is a PR and is in the same repo it might be that the PR contains it's dependency, in that case a review is worth even less. I'm not sure on the color (not green, red or gray because they are taken) and I don't know how hard it would be to do that. Also GitHub doesn't have anything similar. |
One icon instead of the current text imho, to conserve space. Meanwhile I've seen this icon in GitLab suite: https://gitlab.com/gitlab-org/gitlab-svgs/-/blob/main/sprite_icons/issue-block.svg So I would make two icons, similar to that one. Represent the issue/pr with a square and have arrows pointing in and out at bottom right, similar to package octicons above. If you can operate Inkscape or similar tools, please try creating these SVGs. Otherwise I will try 😆. |
…king-in-issue-list
Can someone do this for me? I'm not the best to edit/create SVGs |
Sorry, haven't gotten around to making these SVGs yet, I will soon. |
@zokkis SVGs added. Want to integrate them? I'd say wrap them in a |
{{template "shared/issue_dependency" (dict | ||
"Dependencies" (index $.BlockedByDependenciesMap .ID) | ||
"TitleKey" "repo.issues.dependency.blocked_by_following")}} | ||
{{end}} |
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.
{{end}} | |
{{end}} |
IssueID int64 `xorm:"UNIQUE(issue_dependency) NOT NULL index"` | ||
DependencyID int64 `xorm:"UNIQUE(issue_dependency) NOT NULL index"` |
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 nits:
- Does it need a migration? For long time I have the question:
- If a migration is a must, it should be documented in guideline and explained.
- If a migration is not a must, then we do not need to require to write migrations
- The index for
IssueID
seems redundant, becauseissue_dependency
should already provide the index for it.
In my opinion these are way too similar and hard to tell apart, but I prefer it to be merged, then someone else can raise it in case if it's just me. |
Show depending and blocked PRs/Issues in the PR/Issue list (Resolves #29018) and added a divider between texts in the item-body
before:
after: