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

Fix title behind navbar #815

Merged
merged 6 commits into from
Mar 21, 2023

Conversation

nghuyhoang0204
Copy link
Contributor

Hey i've checked this issue and i think the problem is about the margin-top in CSS of this page so i've added and here is the result
Screenshot 2023-03-15 at 01 04 22

@netlify
Copy link

netlify bot commented Mar 15, 2023

Deploy Preview for acmcsuf ready!

Name Link
🔨 Latest commit 8e56ce6
🔍 Latest deploy log https://app.netlify.com/sites/acmcsuf/deploys/641901a689282c00088ca34a
😎 Deploy Preview https://deploy-preview-815--acmcsuf.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Collaborator

@karnikaavelumani karnikaavelumani left a comment

Choose a reason for hiding this comment

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

Thank you so much for submitting your code changes! I respect your view of simplicity when it comes to this issue. However, adding a specific value for padding can lead to inconsistent and unpredictable results making it harder to reapproach this page in the future for changes.

Instead, we should focus on centering the content itself. While I appreciate your efforts to improve the layout, I think we need to take a more deliberate approach to ensure that the content is centered correctly.

In order to achieve this, I recommend that you take a look at the HTML and consider putting the content in the section tag into a div tag so that it will be easy to center
image

From there you should try to see if it is possible to center it without any numeric values for the sake of preventing brute forced programming.

Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment

Choose a reason for hiding this comment

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

Instead, we should focus on centering the content itself.

The suggestion to center the 404 page content in a new DIV is a valid heuristic.

Note: The DIV might need a height >100vh since a height of 100vh might be too small.

@nghuyhoang0204
Copy link
Contributor Author

Okayy i see what i have to do for this issue . Thanks for your review and detail suggestion
. Ill try to make another Pr 😸

@EthanThatOneKid
Copy link
Owner

EthanThatOneKid commented Mar 16, 2023

. Ill try to make another Pr 😸

You don’t have to make a new PR, this one is fine to move forward with.

@EthanThatOneKid EthanThatOneKid linked an issue Mar 18, 2023 that may be closed by this pull request
@nghuyhoang0204
Copy link
Contributor Author

nghuyhoang0204 commented Mar 20, 2023

Hello guys , so i've made a few changes to this code sorry if my codes are not clean , please check it and give me comment if i have to fix anything else
And here is the result :
Screenshot 2023-03-20 at 01 04 07

Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment

Choose a reason for hiding this comment

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

please check it and give me comment if i have to fix anything else

Nice! I left a few comments.

src/lib/components/button/button.svelte Outdated Show resolved Hide resolved
src/routes/+error.svelte Outdated Show resolved Hide resolved
@karnikaavelumani
Copy link
Collaborator

image
I would take just a look again because a change of yours actually affected the landing page!

@nghuyhoang0204
Copy link
Contributor Author

Okay guys , i see what i've done so this is my newest commit for yours changes requested . I hope it will be fine now 😄
Screenshot 2023-03-20 at 01 04 07

Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment

Choose a reason for hiding this comment

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

Your PR looks good so far. I left a few more comments for you to consider.

src/lib/components/button/button.svelte Outdated Show resolved Hide resolved
src/routes/+error.svelte Outdated Show resolved Hide resolved
src/routes/+error.svelte Outdated Show resolved Hide resolved
@nghuyhoang0204
Copy link
Contributor Author

I am very happy to follow your requests

Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

Thank you for another contribution, @nghuyhoang0204!

@nghuyhoang0204
Copy link
Contributor Author

With my pleasure , thanks for your reviews and your comments 🤓🤩

Copy link
Collaborator

@karnikaavelumani karnikaavelumani left a comment

Choose a reason for hiding this comment

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

LGTM! Great job:)

@EthanThatOneKid EthanThatOneKid enabled auto-merge (squash) March 21, 2023 01:01
@EthanThatOneKid EthanThatOneKid merged commit 9cf7f40 into EthanThatOneKid:main Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

404 page: Title is behind the navbar
3 participants