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

Improve tree not found page #26570

Merged
merged 27 commits into from
Sep 29, 2023
Merged

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Aug 18, 2023

Before:
before

After:
after

In Github:
https://github.com/yp05327/test/blob/main/test.drawio

Updated:
UI changed
image
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 18, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 18, 2023
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

  1. bugs: {{.locale.Tr "repo.tree_not_found" .BranchName .RepoName .TreePath | Safe}}=> XSS
  2. design: I do not think it's maintainable to fill the "404.tmpl" with more and more if blocks. IMO either:
    1. Use a dedicate page for "tree path not found"
    2. Use a general approach for 404 page, eg: "NotFoundPrompt" + "NotFoundGoBackURL"

yp05327 and others added 2 commits August 18, 2023 16:00
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 18, 2023
@silverwind
Copy link
Member

silverwind commented Aug 18, 2023

Agree to a dedicated rendering without the big 404. Maybe just conditionally remove the logo in such a case.

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 21, 2023
@lunny lunny added the topic/ui-interaction Change the process how users use Gitea instead of the visual appearance label Aug 22, 2023
@lunny lunny added this to the 1.21.0 milestone Aug 22, 2023
delvh
delvh previously requested changes Sep 17, 2023
options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
routers/web/repo/helper.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 17, 2023
Comment on lines 29 to 38
refType := ""
switch {
case ctx.Repo.IsViewBranch:
refType = "branch"
case ctx.Repo.IsViewTag:
refType = "tag"
case ctx.Repo.IsViewCommit:
refType = "commit"
}
ctx.Data["NotFoundPrompt"] = ctx.Locale.Tr("repo.tree_path_not_found_" + refType, ctx.Repo.TreePath, url.PathEscape(ctx.Repo.RefName))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@delvh
There's a risk when refType is empty? I'm not sure what will happen when the key word not exists.

Copy link
Member

Choose a reason for hiding this comment

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

Well, in that case, the user will see the message repo.tree_path_not_found_ instead of Path <path> is not contained within <type> <name>.
In which case, we then know that we need to adapt this switch…

Copy link
Member

Choose a reason for hiding this comment

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

But I don't even know how a user would trigger something like that.
What other ref types are there except branches, commits, and tags?

templates/status/404.tmpl Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 28, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 28, 2023
@lunny lunny enabled auto-merge (squash) September 28, 2023 08:25
@lunny lunny disabled auto-merge September 29, 2023 07:09
@lunny lunny enabled auto-merge (squash) September 29, 2023 07:16
@lunny lunny merged commit 3945c26 into go-gitea:main Sep 29, 2023
25 checks passed
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Sep 29, 2023
@yp05327 yp05327 deleted the improve-tree-not-found branch October 23, 2023 01:39
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/ui-interaction Change the process how users use Gitea instead of the visual appearance type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants