-
Notifications
You must be signed in to change notification settings - Fork 51
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
Fix title behind navbar #815
Conversation
✅ Deploy Preview for acmcsuf ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
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
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.
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.
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.
Okayy i see what i have to do for this issue . Thanks for your review and detail suggestion |
You don’t have to make a new PR, this one is fine to move forward with. |
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.
please check it and give me comment if i have to fix anything else
Nice! I left a few comments.
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.
Your PR looks good so far. I left a few more comments for you to consider.
I am very happy to follow your requests |
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.
LGTM! 🎉
Thank you for another contribution, @nghuyhoang0204!
With my pleasure , thanks for your reviews and your comments 🤓🤩 |
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.
LGTM! Great job:)
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