oppia / foundation-website

Repository for developing the new Oppia Foundation website.
Apache License 2.0
6 stars 32 forks source link

About Foundation page #121

Closed hoangviet1993 closed 5 years ago

hoangviet1993 commented 5 years ago

This PR revamp the current About page to provide more details about the Foundation in general.

Changes in this PR are live at: https://oppia-foundation-test-server.appspot.com/about

Some screenshots Desktop (1500px): screencapture-oppia-foundation-test-server-appspot-about-2019-08-06-16_32_38

Tablet (iPad 2 horizontal rotation): screencapture-oppia-foundation-test-server-appspot-about-2019-08-06-16_33_32

Mobile (ip6/7/8): screencapture-oppia-foundation-test-server-appspot-about-2019-08-06-16_33_07

hoangviet1993 commented 5 years ago

@rachelwchen @dchen97 Can you PTAL?

dchen97 commented 5 years ago

Thanks so much Viet! Just a few notes:

Other than those notes, it looks great to me!

codecov-io commented 5 years ago

Codecov Report

Merging #121 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #121   +/-   ##
=======================================
  Coverage   96.41%   96.41%           
=======================================
  Files          21       21           
  Lines         390      390           
=======================================
  Hits          376      376           
  Misses         14       14

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 631a0a3...14463f4. Read the comment docs.

hoangviet1993 commented 5 years ago

@dchen97 All done! Thanks

hoangviet1993 commented 5 years ago

we need to update the links for the buttons on the homepage to now appropriately direct to either this page or the about Oppia page.

@dchen97 This is already done in this PR. Can you visit the test site and take a look at the navbar on desktop and the sidebar on mobile?

dchen97 commented 5 years ago

Ahh I was speaking more on the highlighted buttons on the homepage, like the one that says "See the Oppia Difference." I think the navbar is fine.

On Thu, Aug 8, 2019, 12:18 PM Viet Tran Quoc Hoang notifications@github.com wrote:

we need to update the links for the buttons on the homepage to now appropriately direct to either this page or the about Oppia page.

This is already done in this PR. Can you visit the test site and take a look at the navbar on desktop and the sidebar on mobile?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/oppia/foundation-website/pull/121?email_source=notifications&email_token=AC32BWZ45YK5FORD7C4QQZTQDRIOFA5CNFSM4IJ3BXSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD34J6ZI#issuecomment-519610213, or mute the thread https://github.com/notifications/unsubscribe-auth/AC32BW7TNQOWMCUJVVIKG6LQDRIOFANCNFSM4IJ3BXSA .

hoangviet1993 commented 5 years ago

@dchen97 Ah, got it! Mb for not reading your comment clearly there, sorry! I have changed the link of the "See the Oppia Difference" button.

rachelwchen commented 5 years ago

Thanks @hoangviet1993!

Two comments:

  1. For the section titles, the character spacing seems too tight ("Millions of children are not..."). Can we increase the spacing a bit please? The same goes for the rest of the page ("We cannot ignore the potential quality...," "Creating fun and effective learning for...," and so on.) image

  2. In the section, "Millions of children are not..." can we add a line of text below the infographic that links to the source? @dchen97 if you have the URL that'd be great :) image

hoangviet1993 commented 5 years ago

For the section titles, the character spacing seems too tight ("Millions of children are not..."). Can we increase the spacing a bit please? The same goes for the rest of the page

Done here and elsewhere!

In the section, "Millions of children are not..." can we add a line of text below the infographic that links to the source?

@rachelwchen I have added the source for the infographics. Can you PTAL again?

hoangviet1993 commented 5 years ago

@seanlip Can you PTAL?

seanlip commented 5 years ago

Looks good code-wise -- thanks @hoangviet1993!

I took a quick look at the demo page and I have one question. In the image at the bottom (the hub with 5 spokes showing 5 contributors), why is Anmol's photo repeated? We should replace it with an image that shows 5 unique people.

rachelwchen commented 5 years ago

Hey Viet! In the "There's a role for you to play" section, can we increase the character spacing as well please? Right now it looks like it's about -0.5 to -0.6 -- I think it'll be more readable if it's at 0.0 :) thanks! Once that's done then I think we're ready! image

rachelwchen commented 5 years ago

Re: Sean's comment on the picture having duplicates -- in the "Built from a unified passion for educational equality" section, please use the image below instead. Thank you! Artboard 6

hoangviet1993 commented 5 years ago

@rachelwchen I have deployed your image and increased the letter spacing like you suggested. PTAL at https://oppia-foundation-test-server.appspot.com/about

rachelwchen commented 5 years ago

Thanks @hoangviet1993! I think the letter spacing is still a little tight in the last section, can we set it to 0 please: image

hoangviet1993 commented 5 years ago

@rachelwchen All done!

seanlip commented 5 years ago

LGTM, please feel free to merge once tests pass. Thanks @hoangviet1993!