hoangviet1993 commented 5 years ago

[x] Update hero card and put in new instructions for partnerships card.
[x] Remove old codes and old assets.
[x] Minimize new hero pictures.

Changes in this PR are live at https://oppia-foundation-test-server.appspot.com/partnerships Here are some screenshots:

Mobile (ip5/6): partnerships_page_mobile

Tablet (iPad): partnerships_page_tablet

Desktop: partnerships_page_desktop

codecov-io commented 5 years ago

Codecov Report

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

@@           Coverage Diff           @@
##           master     #116   +/-   ##
=======================================
  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 ff53f3a...3961bcd. Read the comment docs.

hoangviet1993 commented 5 years ago

@rachelwchen Can you also help me review the new Partnerships page at https://oppia-foundation-test-server.appspot.com/partnerships ?

rachelwchen commented 5 years ago

Hi @hoangviet1993 -- thanks for implementing this!!

Three comments: 1 -- can we increase the left margin, so that the paragraph aligns with the rest of the page please? (See [1] with the dotted red lines) 2 -- let's decrease the spacing between the number and the text a little (see [2]) 3 -- let's adjust the position a bit, so that each image and text align with each individual column (see [3]).

For reference: Current -- Desktop-cur

Changes to be made -- Desktop-edit

hoangviet1993 commented 5 years ago

Hi @rachelwchen I have made some changes according to your requests. Please let me know what you think!

2 -- let's decrease the spacing between the number and the text a little (see [2])

Done!

3 -- let's adjust the position a bit, so that each image and text align with each individual column (see [3]).

I have made the item text center aligned. I think it helps, what do you think?

1 -- can we increase the left margin, so that the paragraph aligns with the rest of the page please? (See [1] with the dotted red lines)

I made the hero textbox center aligned instead of left align like. I initially went with your suggestion (1) but I do not think it looks that much better in comparison to center aligning it(2).

(1)Left-align hero text box screencapture-localhost-8080-partnerships-2019-07-15-18_23_24

(2)Center-align hero text box Large view-width (>1700px) screencapture-localhost-8080-partnerships-2019-07-15-19_18_03 Medium view-with (1355px)

Again, changes are live on: https://oppia-foundation-test-server.appspot.com/partnerships

rachelwchen commented 5 years ago

Thanks for getting this done so quickly!

For the large-width view, is it possible to set the width for the text, so the text for (2) doesn't look too much "longer" than the other ones? Not sure if this will be a troublesome fix -- if so, feel free to disregard this :) otherwise I think it's good if other folks don't have further comments!

dchen97 commented 5 years ago

Thanks for all the hard work on this! If all of the above comments from Rachel are resolved, then it LGTM as well!

hoangviet1993 commented 5 years ago

For the large-width view, is it possible to set the width for the text, so the text for (2) doesn't look too much "longer" than the other ones?

@rachelwchen Done! Thank you so much for the clear explanations. @seanlip can you PTAL?

seanlip commented 5 years ago

Thanks a lot! Looks great IMO. Just one question, can you make the names of all the icon files lowercase? After that it's good to go.

hoangviet1993 commented 5 years ago

@seanlip I have renamed the icon files and made the necessary adjustments. Can you PTAL?

seanlip commented 5 years ago

Hi @hoangviet1993, I'm having trouble reviewing the diff for the PR since my last review. Why did you need to force-push? That tends to overwrite history, we generally discourage it.

hoangviet1993 commented 5 years ago

I was trying to squash the file-renaming together with another commit which changes a JS file. Do you want me to revert the force push?

seanlip commented 5 years ago

Let's leave it for now -- I'll take a full pass again. But next time, please prefer making multiple commits rather than doing any force-pushing -- that will make it possible for reviewers to review the diff since they last reviewed. Thanks!

(Also, in general, never force-push for this and other Oppia repos -- it can be destructive, as we've found in the past. Suggest asking in advance if you have a special reason to do so.)

hoangviet1993 commented 5 years ago

Got it @seanlip. So sorry for the trouble since the forced push was over menial stuff. I will simply keep committing like usual.

seanlip commented 5 years ago

No problem! Thanks @hoangviet1993 :)

oppia / foundation-website

Fix #115: Partnerships page redesign #116

Codecov Report