oppia / foundation-website

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

Volunteer page redesign #122

Closed hoangviet1993 closed 4 years ago

hoangviet1993 commented 5 years ago

This PR seeks out to match the current page to the provided mock.

I am currently seeking out designer's opinion on some changes so this PR only includes straight-forward changes that can be completed easily.

Changes are live on: https://oppia-foundation-test-server-1.appspot.com/volunteer

Some screenshots Desktop view: screencapture-oppia-foundation-test-server-1-appspot-volunteer-2019-12-20-16_29_56

Tablet view (iPad): screencapture-oppia-foundation-test-server-1-appspot-volunteer-2019-12-20-16_31_22

Mobile view (ip6/7/8): screencapture-oppia-foundation-test-server-1-appspot-volunteer-2019-12-20-16_32_09

codecov-io commented 5 years ago

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #122   +/-   ##
=======================================
  Coverage   96.59%   96.59%           
=======================================
  Files          21       21           
  Lines         411      411           
=======================================
  Hits          397      397           
  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 fb5934e...37c341b. Read the comment docs.

rachelwchen commented 5 years ago

Thank you!! Two comments:

  1. In mobile view, can we add more spacing between the map and the buttons? See below: image

  2. The bottom "Get In Touch" section -- for the sentence, "Wondering if your skills can help Oppia grow?" can we change the if to how so that it says "Wondering how your skills can help Oppia grow?"

hoangviet1993 commented 5 years ago

Changes are live at https://oppia-foundation-test-server-1.appspot.com/volunteer

In mobile view, can we add more spacing between the map and the buttons

I have added some padding between the image the button rows. Can you take a look again?

"Wondering if your skills can help Oppia grow?" can we change the if to how so that it says "Wondering how your skills can help Oppia grow?"

Done!

@rachelwchen I have addressed your comments. Currently I am waiting for Chantel to finish up re-drafting his mock again.

seanlip commented 4 years ago

Hi @hoangviet1993 @dchen97 -- quick question, what is the current status of this PR?

Is it ready for merge, or @hoangviet1993, are you waiting on further input?

Thanks!

hoangviet1993 commented 4 years ago

@rachelwchen I have made some progress with regards to the new design. I would need some more time to match the new carousel look. Latest changes are live at: https://oppia-foundation-test-server-1.appspot.com/volunteer

hoangviet1993 commented 4 years ago

@seanlip and @dchen97 I'm so sorry for coming in with this so late. I have tried to match Chantel latest mock in the email thread. Latest changes are live at https://oppia-foundation-test-server-1.appspot.com/volunteer I have also checked for cross-browser issues on FF and Safari. PTAL and give me your feedbacks!

seanlip commented 4 years ago

Hi @hoangviet1993, thanks! I just did a click-around on the test page. Here are some notes:

hoangviet1993 commented 4 years ago

@seanlip Thanks for the feedbacks. Can you clarify a few things for me?

The pictures of the volunteers seem weird -- they seem too "compressed" and a bit squashed. Do you feel that too?

I am aware of this issue. Sam and Joe photos came out fine while other carousel picture became distorted. I have not had the time investigate properly but think I need to resize the photos to match the actual element size, which is much smaller than the actual photo size. I need to treat each photo individually though. Will come back to this once all the other code-related changes are in place!

The section below the carousel always starts with "Volunteer Developer", regardless of the Carousel section, and that seems a little weird. What do you think about changing that text so that it only shows up for developers (with the GitHub button) and for all other rows it shows the interest form?

On Desktop, where user can select a tab, we can conditionally display the "Go to Github" button, remove some text from the paragraph, etc... whenever the user navigate away from the Development tab. However, on mobile and tablet (<960px view-width), the tab goes away and all 4 types of volunteers are displayed. I assume we are keeping things as is on mobile?

The current last sentence for the blurb below the carousel does not end with a period.

Thanks for uncovering this :) I added periods elsewhere.

On larger-width screens the blurb below is far to the left. Could it be left-aligned within a horizontally-centered div with at most 700-800 px width?

I am having trouble replicating this. Can you provide screenshot? I did have the "call-to-action" section in a container of max 850px width and then centering that container horizontally.

seanlip commented 4 years ago

Thanks, @hoangviet1993!

Re the pictures -- I wonder whether it might help to make them larger somehow. Currently they are quite small and I'm not sure resizing them to be smaller would be better. Might it be worth working with the designers to see if we can show a larger picture?

Hm, good point about mobile. OK, how about this instead -- let's have the text point towards the volunteer form in all cases, with a parenthetical for GitHub. E.g.: "If you're interested in helping out, simply let us know what you're interested in working on and a lead will reach out to you shortly! You can fill out the form below (or go to GitHub(LINK) if you're interested in contributing to the codebase as a developer)." -- something like that?

For the last point, here's what I see when I zoom the page out:

Screenshot from 2020-02-17 00-00-58

As you can see, the text at the bottom is really far to the left. Maybe 850px is too wide? (Not sure.)

hoangviet1993 commented 4 years ago

Re the pictures -- I wonder whether it might help to make them larger somehow. Currently they are quite small and I'm not sure resizing them to be smaller would be better. Might it be worth working with the designers to see if we can show a larger picture?

I can certainly make the carousel profile picture element larger. How does it look now?

Hm, good point about mobile. OK, how about this instead -- let's have the text point towards the volunteer form in all cases, with a parenthetical for GitHub. E.g.: "If you're interested in helping out, simply let us know what you're interested in working on and a lead will reach out to you shortly! You can fill out the form below (or go to GitHub(LINK) if you're interested in contributing to the codebase as a developer)." -- something like that?

Done!

For the last point, here's what I see when I zoom the page out:

Ah, I did not zoom out. Apologies! I needed to also horizontally center the container div as well as I only center-ed the text div. Anyhow, this should be fixed?

I have deployed the latest changes to the testing server. PTAL @seanlip

seanlip commented 4 years ago

I think it looks great, thanks @hoangviet1993! I think the only issue is the pictures, which still look pretty "off" and do need to be fixed before deploying IMO. I also wonder if there's a design where they can be bigger, similar to the current one, since (for me) a main highlight of this section is the awesome people involved in the project :) Other than that, I have no other comments.

@dchen97 -- do you have any feedback on @hoangviet1993's implementation?

dchen97 commented 4 years ago

Just a small nit that the you' and the re in "simply let us know what you're interested in" seems to have an unnecessary space.

I don't know if this is just my computer, but when I go to the different tabs, sometimes the role description text gets a bit blurry, which I've tried to illustrate below: Screen Shot 2020-02-19 at 3 46 11 PM

If @seanlip or @hoangviet1993 know how to fix this quickly, that would be great. Otherwise, I don't think it should block this launch by more than 1-2 weeks.

hoangviet1993 commented 4 years ago

sometimes the role description text gets a bit blurry

@dchen97 I think I fix the blurry text issue. Can you help me check if it is true? Kudos to the good catch again 👍

dchen97 commented 4 years ago

LGTM besides the photo issue that @seanlip noted above!

hoangviet1993 commented 4 years ago

@seanlip Are we going back to the designers to get a new profile carousel design?

seanlip commented 4 years ago

Could you ask them about the photos issue? I think that's the only thing, I don't think the entire carousel needs to be redesigned.

hoangviet1993 commented 4 years ago

I have tried to fix the distorted profile image issue by using background-img. I think it helped. @dchen97 Who should I be connecting with? Chantel? @seanlip How does the profile carousel's images look now?

seanlip commented 4 years ago

Thanks, @hoangviet1993! It does look better now -- not so squashed :) I think the only remaining thought I have is about their size (as mentioned above).

And yep, Chantel would probably be the best person to talk to! If you loop me in on the thread, I can also help provide additional context where needed.

hoangviet1993 commented 4 years ago

@seanlip How does it look now?

seanlip commented 4 years ago

I think it looks fantastic :) Thanks, @hoangviet1993!

Just one comment. Could you please have the GitHub link go to https://github.com/oppia/oppia/wiki instead? I think that's a better avenue for new contributors.

Thanks!

hoangviet1993 commented 4 years ago

Just one comment. Could you please have the GitHub link go to https://github.com/oppia/oppia/wiki instead? I think that's a better avenue for new contributors.

Just the link on the volunteer page right? Inside the Development tab and the Development card on mobile, there is also a link going to the Oppia codebase. Would you want to change those too?

seanlip commented 4 years ago

Yes, let's have all links go to the wiki page. (It's easy to find the homepage from the wiki page, but the other way round is less easy.) Thanks!

hoangviet1993 commented 4 years ago

Yes, let's have all links go to the wiki page. (It's easy to find the homepage from the wiki page, but the other way round is less easy.) Thanks!

Done!

seanlip commented 4 years ago

Cool! Thanks -- LGTM from my end :)

hoangviet1993 commented 4 years ago

@rachelwchen Can you PTAL?

hoangviet1993 commented 4 years ago

Diana gave a LGTM via email. Screen Shot 2020-02-26 at 10 59 01 AM @seanlip @dchen97 Is Chantel reachable on Github?

seanlip commented 4 years ago

Yep! /cc @mschanteltc

seanlip commented 4 years ago

@mschanteltc approved, so I'm merging this. Thanks so much for the PR, @hoangviet1993!