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.

Sign up for GitHub

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

Jump to bottom

UI: Changed the error page and modified the 404 page #4032

Merged
merged 1 commit into from
Mar 26, 2025
Merged

UI: Changed the error page and modified the 404 page #4032

merged 1 commit into from
Mar 26, 2025

Conversation

SINF-KEN
Copy link
Contributor

@SINF-KEN SINF-KEN commented Mar 21, 2025 •
edited
Loading

Co-authored-by: notnotnescap [email protected]

  • Changed the error page, so that it looks better feedback from discord and the artist ( 96a5b97 )

  • Modified the error/crash page so it looks more in line with the 404 page and the brand guidelines.

@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@SINF-KEN SINF-KEN changed the title UI: changed the error page and modified the 404 page UI: Changed the error page and modified the 404 page Mar 21, 2025
@SINF-KEN SINF-KEN force-pushed the UI--new-error-page branch 2 times, most recently from 8054917 to 494b923 Compare March 21, 2025 11:31
@SINF-KEN
Copy link
Contributor Author

SINF-KEN commented Mar 21, 2025 •
edited
Loading

image image image image

@SINF-KEN
Copy link
Contributor Author

any feedback here would be appreciated (still new to the project)

AtkinsSJ
AtkinsSJ requested changes Mar 26, 2025
View reviewed changes
Copy link
Member

@AtkinsSJ AtkinsSJ left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I have a couple of comments but they don't easily fit in the diff itself so I'll put them here.

First, it'd be nice if the two error pages had the same spacing between the image and the title.

The code itself is fine! I think eventually it makes sense to move the crash page HTML into its own file like we already have for error.html, but keeping it in the C++ is fine for now.

For the commit though, it's somehow marked as authored by Tim instead of you. You can undo the commit while keeping your changes, with git reset --mixed HEAD~1, and then commit them again.

Also the message should be more descriptive and be in the present tense. eg, something like this:

UI: Style the crash page and update error page image

Add styling and an image to the crash page, so that it matches the
error page. Also update the error page's image to have a thinner
stroke.

Co-authored-by: notnotnescap <[email protected]>

@AtkinsSJ
Copy link
Member

  • Modified the error/crash page so it looks more in line with the 404 page and the brand guidelines.

Oh I just noticed you have this in the PR message! This is the sort of thing that's great to include in the commit message itself.

@SINF-KEN SINF-KEN force-pushed the UI--new-error-page branch 4 times, most recently from 5eb91b7 to 3ff79c1 Compare March 26, 2025 12:59
@SINF-KEN SINF-KEN requested a review from AtkinsSJ March 26, 2025 13:03
@SINF-KEN SINF-KEN force-pushed the UI--new-error-page branch 3 times, most recently from d2eab83 to 3299762 Compare March 26, 2025 15:50
@SINF-KEN @notnotnescap
UI: Style the crash page and update error page image
fd80d3d 
- Modified the error page
- changed the error/crash page

Co-authored-by: notnotnescap <[email protected]>
@SINF-KEN SINF-KEN force-pushed the UI--new-error-page branch from 4d13cb0 to fd80d3d Compare March 26, 2025 15:58
@SINF-KEN
Copy link
Contributor Author

fixed

AtkinsSJ
AtkinsSJ approved these changes Mar 26, 2025
View reviewed changes
Copy link
Member

@AtkinsSJ AtkinsSJ left a comment

Choose a reason for hiding this comment

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

Great! Don't worry, git gets easier after a while.

As a future suggestion, consider splitting things up into more commits. For example, this one really does two things that are separate from each other.

@AtkinsSJ AtkinsSJ merged commit 1a34676 into LadybirdBrowser:master Mar 26, 2025
7 checks passed
@SINF-KEN
Copy link
Contributor Author

@AtkinsSJ

IDK how to say this, but when I did the rebase, some of my code didn't sync / save and the page looks off / wrong (PR 4032), and it got merged ( sorry i asked to be merged 😅 )
can i make a new PR to fix it? ( or is there a faster way ? thank you and sorry )

@SINF-KEN
Copy link
Contributor Author

SINF-KEN commented Mar 26, 2025 •
edited
Loading

the fixed version

diff --git a/Libraries/LibWebView/ViewImplementation.cpp b/Libraries/LibWebView/ViewImplementation.cpp
index 16095430fd..98edefc2e5 100644
--- a/Libraries/LibWebView/ViewImplementation.cpp
+++ b/Libraries/LibWebView/ViewImplementation.cpp
@@ -648,11 +648,12 @@ void ViewImplementation::handle_web_content_process_crash(LoadErrorPage load_err
                        "p { font-size: 1rem; color: #555; }"
                        "</style></head><body>"sv);
         builder.append("<header>"sv);
-        builder.append("<svg id=\"a\" xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 17.5 21.5\">"sv);
+        builder.append("<svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 17.5 21.5\">"sv);
         builder.append("<path class=\"b\" d=\"M11.75.75h-9c-1.1 0-2 .9-2 2v16c0 1.1.9 2 2 2h12c1.1 0 2-.9 2-2v-13l-5-5z\"/>"sv);
         builder.append("<path class=\"b\" d=\"M10.75.75v4c0 1.1.9 2 2 2h4M4.75 9.75l2 2M10.75 9.75l2 2M12.75 9.75l-2 2M6.75 9.75l-2 2M5.75 16.75c1-2.67 5-2.67 6 0\"/></svg>"sv);
         auto escaped_url = escape_html_entities(m_url.to_byte_string());
-        builder.appendff("The web page <a href=\"{}\">{}</a> has crashed.<br><br>You can reload the page to try again.", escaped_url, escaped_url);
+        builder.append("<h1>Ladybird flew off-course!</h1>"sv);
+        builder.appendff("<p>The web page <a href=\"{}\">{}</a> has crashed.<br><br>You can reload the page to try again.</p>", escaped_url, escaped_url);
         builder.append("</body></html>"sv);
         load_html(builder.to_byte_string());
     }

@AtkinsSJ
Copy link
Member

@AtkinsSJ

IDK how to say this, but when I did the rebase, some of my code didn't sync / save and the page looks off / wrong (PR 4032), and it got merged ( sorry i asked to be merged 😅 ) can i make a new PR to fix it? ( or is there a faster way ? thank you and sorry )

No worries. If you're happy to make a new PR, that'd be good!

@SINF-KEN SINF-KEN mentioned this pull request Mar 26, 2025
Merged
@SINF-KEN
Copy link
Contributor Author

@AtkinsSJ
IDK how to say this, but when I did the rebase, some of my code didn't sync / save and the page looks off / wrong (PR 4032), and it got merged ( sorry i asked to be merged 😅 ) can i make a new PR to fix it? ( or is there a faster way ? thank you and sorry )

No worries. If you're happy to make a new PR, that'd be good!

sorry again :/
#4112

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AtkinsSJ AtkinsSJ AtkinsSJ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SINF-KEN @ladybird-bot @AtkinsSJ