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

Simplify 404/500 page #31409

Merged
merged 14 commits into from
Jun 23, 2024
Merged

Simplify 404/500 page #31409

merged 14 commits into from
Jun 23, 2024

Conversation

wxiaoguang
Copy link
Contributor

It doesn't need to waste user's bandwidth to download a 404.png/500.png when error occurs.

ps: I think we do not need to translate "Not Found" and "Internal Server Error", they are widely used internet terms IMO, and they are clearer than the previous images (which only show a number)

image

image

image

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 18, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 18, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Jun 18, 2024
@wxiaoguang wxiaoguang added the type/enhancement An improvement of existing functionality label Jun 18, 2024
@wxiaoguang wxiaoguang added this to the 1.23.0 milestone Jun 18, 2024
@silverwind
Copy link
Member

silverwind commented Jun 18, 2024

Maybe put on two lines?

       404 (48px)
    Not Found (32px)

@wxiaoguang
Copy link
Contributor Author

Maybe put on two lines?

       404 (48px)
    Not Found (32px)

No idea which looks really better. Fee free to edit it.

Personally I don't think it's worth to make it that complex, most users do not use these pages daily.

@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 Jun 19, 2024
@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 Jun 19, 2024
web_src/css/base.css Outdated Show resolved Hide resolved
@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 Jun 19, 2024
@silverwind
Copy link
Member

silverwind commented Jun 19, 2024

Some tweaks done. Ideally I'd want to vertically center on screen but that's actually not so cleanly possible with the current CSS so I left it unchanged. New design has less space and no ugly divider on 500 page.

Screenshot 2024-06-20 at 01 44 33 Screenshot 2024-06-20 at 01 44 24 Screenshot 2024-06-20 at 01 44 43

@silverwind
Copy link
Member

Actually changed the color to --color-text-light-2:

image

@wxiaoguang
Copy link
Contributor Author

Wrong position.

image

@silverwind
Copy link
Member

Fixed, the asymmetry is ugly, but I guess that text above would need to be separated out of the <pre> block to fix that as well.

image

@silverwind
Copy link
Member

silverwind commented Jun 20, 2024

image

Maybe reduce spacing below code a small bit so that margin looks more equal above and below?

@silverwind
Copy link
Member

Tweaked once more. I think it looks more visually pleasing if vertical margin between elements is roughly the same.

Screenshot 2024-06-20 at 23 48 56 Screenshot 2024-06-20 at 23 48 44

@6543 6543 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 20, 2024
@silverwind silverwind removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 20, 2024
@silverwind
Copy link
Member

Hold on with merge, I will try to vertically center on screen.

@silverwind
Copy link
Member

Vertical centering added. Took a bit more CSS than I would have liked but the page layout is quite complex unfortunately.

Screenshot 2024-06-21 at 00 14 03 Screenshot 2024-06-21 at 00 14 27

@silverwind
Copy link
Member

Let's wait for @wxiaoguang to have a final look.

@wxiaoguang
Copy link
Contributor Author

Vertical centering added. Took a bit more CSS than I would have liked but the page layout is quite complex unfortunately.

I really dislike using too many CSS tricks for such a trivial page.

@wxiaoguang wxiaoguang marked this pull request as draft June 20, 2024 23:03
@wxiaoguang
Copy link
Contributor Author

And it is even totally wrong! I will revert all unnecessary changes and won't spend time on such a trivial PR.

image

@wxiaoguang wxiaoguang marked this pull request as ready for review June 20, 2024 23:14
@wxiaoguang wxiaoguang enabled auto-merge (squash) June 20, 2024 23:14
@silverwind
Copy link
Member

silverwind commented Jun 20, 2024

I really dislike using too many CSS tricks for such a trivial page.

As I said, the complex CSS is because the page layout is bad and could still drastically simplified.

And it is even totally wrong! I will revert all unnecessary changes and won't spend time on such a trivial PR.

Would have been a trivial fix. I will iterate on it and vertically center it. I think it's important to have well-designed pages and especially on big screen it looks bad with the fixed margin currently.

@silverwind silverwind self-requested a review June 20, 2024 23:29
@wxiaoguang
Copy link
Contributor Author

I really dislike using too many CSS tricks for such a trivial page.

As I said, the complex CSS is because the page layout is bad and could still drastically simplified.

Hacking tricks don't really work well. There are different cases:

  • 404 full page
  • 404 embedded page with repo header
  • 500 full page
  • 500 in a partially rendered page, the error message part could appear at any place, unpredictable.

And it is even totally wrong! I will revert all unnecessary changes and won't spend time on such a trivial PR.

Would have been a trivial fix. I will iterate on it and vertically center it. I think it's important to have well-designed pages and especially on big screen it looks bad with the fixed margin currently.

I believe in: correctness > good maintainability > feature/enhancement > base maintainability > unclear patching for blockers > tricky fine-tunes.

For most UI alignment problems, we have fixed them with "base maintainability". But for this one, it only seems to be "tricky fine-tunes" at the moment.

@wxiaoguang
Copy link
Contributor Author

If you would really to fine tune, try this: cb1462d

image

image

@wxiaoguang
Copy link
Contributor Author

Anything else to change? If no, leave more fune-tunes to the future.

At least, this change is already better than before and doesn't introduce new problems (the UI didn't align in old code either)

@wxiaoguang wxiaoguang merged commit f4921b9 into go-gitea:main Jun 23, 2024
26 checks passed
@wxiaoguang wxiaoguang deleted the improve-error-page branch June 23, 2024 17:48
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 24, 2024
* giteaofficial/main:
  Disable issue/PR comment button given empty input (go-gitea#31463)
  Simplify 404/500 page (go-gitea#31409)
  Fix web notification icon not updated once you read all notifications (go-gitea#31447)
  Switch to "Write" tab when edit comment again (go-gitea#31445)
  Add simple JS init performance trace (go-gitea#31459)
@silverwind
Copy link
Member

The centering looks slightly off, maybe I will tune later.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants