nodejs / nodejs.org

The Node.js® Website

https://nodejs.org

MIT License

6.14k stars 6.21k forks source link

fix: Footer added #6830

Closed Wellitsabhi closed 2 months ago

Wellitsabhi commented 3 months ago

Description

Added Footer in 'Learn' and 'About' section

Validation

Screenshot_20240611_115955
Screenshot_20240611_115708

Related Issues

Fixes #6829

Check List

[x] I have read the Contributing Guidelines and made commit messages that follow the guideline.
[x] I have run npm run format to ensure the code follows the style guide.
[x] I have run npm run test to check if all tests are passing.
[x] I have run npx turbo build to check if the website builds without errors.
[x] I've covered new added functionality with unit tests if necessary.

vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
nodejs-org	✅ Ready (Inspect)	Visit Preview	Jun 21, 2024 6:38pm

github-actions[bot] commented 3 months ago

Lighthouse Results URL	Performance	Accessibility	Best Practices	SEO	Report
/en	🟢 99	🟢 100	🟢 100	🟢 91	🔗
/en/about	🟢 100	🟢 100	🟢 100	🟢 91	🔗
/en/about/previous-releases	🟢 100	🟢 100	🟢 100	🟢 92	🔗
/en/download	🟢 99	🟢 100	🟢 96	🟢 91	🔗
/en/blog	🟢 99	🟢 100	🟢 100	🟢 92	🔗

github-actions[bot] commented 3 months ago

Unit Test Coverage Report

Lines	Statements	Branches	Functions
	90.67% (593/654)	76.08% (175/230)	94.57% (122/129)

Unit Test Report

Tests	Skipped	Failures	Errors	Time
131	0 :zzz:	0 :x:	0 :fire:	5.257s :stopwatch:

AugustinMauroy commented 3 months ago

Ok good, but you need to do something ´cause actually the footer take to much place on mobile screen.

Wellitsabhi commented 3 months ago

Ok , will add css for small screens only

Wellitsabhi commented 3 months ago

My finding is issue only persists where ArticleLayout comp is used. For check - I inserted dummy data in /about , and was working fine , no sticky footer

https://github.com/nodejs/nodejs.org/assets/99867024/f64f3e36-0428-4be2-8412-1be64446afba

Wellitsabhi commented 3 months ago

applied external css on \ , on small screen, it will appear above breadcrumb. otherwise working noramlly. UI looks good too.

https://github.com/nodejs/nodejs.org/assets/99867024/b0d3808b-16ed-422b-864a-9b9a1d0e2d9b

Wellitsabhi commented 3 months ago

@bmuenzenmeyer @abizek @canerakdas Can you please review my latest commit to consider it for merge. Let me know if it needs any changes. Thanks.

abizek commented 3 months ago

LGTM

ovflowd commented 3 months ago

@bmuenzenmeyer I dismissed your review, but feel free to do a re-review :pray:

ovflowd commented 3 months ago

Hmmm, the footer is still sticky, and we don't really want that as it means the overall content available for the page itself is diminished...

Wellitsabhi commented 3 months ago

@ovflowd which layout will be good? (below)

height- 100vh ( has better responsivity in medium devices too)

https://github.com/nodejs/nodejs.org/assets/99867024/9cae63a1-9d3e-4538-b1fe-d308371e8bbe

height -100%

https://github.com/nodejs/nodejs.org/assets/99867024/a30a6fa3-1e29-413b-9a4d-0fe58b35c764

I hope it address sticky issue.

bmuenzenmeyer commented 2 months ago

This PR will need to be rebased or recreated now that https://github.com/nodejs/nodejs.org/pull/6850 merged.

Wellitsabhi commented 2 months ago

I shall create new PR. But it would be great if someone can suggest which layout should i follow