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/adjust navigation bar and font size in layout #239

Merged
merged 6 commits into from
Sep 23, 2023

Conversation

ljc1991
Copy link
Collaborator

@ljc1991 ljc1991 commented Sep 22, 2023

Description

  • Adjust navigation bar
  • Adjust font-size in text

minor

  • remove page content navigation in activities page and cards page
  • change font-size from 14px to 16px in text of section
  • (inactive) add dynamic page navigation list

Impaction

Screenshots

Scenarios Before After
NavBar 截圖 2023-09-22 13 52 41 截圖 2023-09-22 13 52 49
font-size 截圖 2023-09-22 13 53 06 截圖 2023-09-22 13 53 20

Performance

Build and deploy time

N/A

LightHouse

N/A

@ljc1991 ljc1991 requested a review from ben196888 September 22, 2023 06:11
@netlify
Copy link

netlify bot commented Sep 22, 2023

Deploy Preview for openstartervillage-canary ready!

Name Link
🔨 Latest commit 51575ef
🔍 Latest deploy log https://app.netlify.com/sites/openstartervillage-canary/deploys/650e92cf5019f40008eac220
😎 Deploy Preview https://deploy-preview-239--openstartervillage-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 88 (no change from production)
Accessibility: 95 (no change from production)
Best Practices: 83 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@netlify
Copy link

netlify bot commented Sep 22, 2023

Deploy Preview for openstartervillage ready!

Name Link
🔨 Latest commit 51575ef
🔍 Latest deploy log https://app.netlify.com/sites/openstartervillage/deploys/650e92cf9a26bd0008336f58
😎 Deploy Preview https://deploy-preview-239--openstartervillage.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 86 (🟢 up 3 from production)
Accessibility: 95 (no change from production)
Best Practices: 83 (🔴 down 9 from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@ben196888
Copy link
Collaborator

Please remove the unused CSS instead of comment them out.

@ben196888
Copy link
Collaborator

It's very good to have dynamic navigation links. 🥳 Nice work!

May I know your reason not to replace the hard code approach?

@ljc1991
Copy link
Collaborator Author

ljc1991 commented Sep 22, 2023

Please remove the unused CSS instead of comment them out.

Yes, Sir!

@ljc1991
Copy link
Collaborator Author

ljc1991 commented Sep 22, 2023

It's very good to have dynamic navigation links. 🥳 Nice work!

May I know your reason not to replace the hard code approach?

I think it will be better to wait until the content for the resource page is completely updated.
BTW, I haven't added the links to the cards page and activities page in the dynamic navigation list yet...

1. Add filter to check if the path and name in each page are empty
2. Add a hard-coded link to cards page
3. Remove the link to activities page
4. Replace page navigation list with dynamic page navigation list
@ben196888
Copy link
Collaborator

There are some duplicate lines of code on generating the navigation list. Please create a shared function and reuse it.

@ben196888
Copy link
Collaborator

Good work!!!

@ben196888 ben196888 merged commit 9cc83ab into main Sep 23, 2023
8 checks passed
@ben196888 ben196888 deleted the fix/adjust-navigation-bar-and-font-size-in-layout branch October 17, 2023 03:36
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.

2 participants