Closed hoangviet1993 closed 5 years ago
Merging #106 into master will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master #106 +/- ##
=======================================
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 07a061f...f2bffe8. Read the comment docs.
Thanks so much, Viet! Some comments:
Can we make the three boxes (discrimination, lack of resources, conflict) equal width please?
The bolded text quote seems heavy -- can we unbold them and use the regular weight?
I think we can increase the width of the big rectangle :) Also, in the "ideal for," can we make it a true list format please? Right now, when the text bleeds to the second line, the second line doesn't align with where the first line of text starts.
Looking at this, I’m not really sure what subscribe means - maybe “sign up for newsletter” would be clearer!
Can we make the three boxes (“conducting randomized controlled trials”, “funding our best lesson creators,” and “scaling our impact and outreach”) equal width please? Right now, “conducting randomized controlled trials” is wider than “scaling our impact our outreach.”
Looking at this, I’m not really sure what subscribe means - maybe “sign up for newsletter” would be clearer!
@rachelwchen "Sign up for newsletter" would make the 'Subscribe' a two-line button on mobile. How does "Sign Up" sound? Is that clear enough?
I think we can increase the width of the big rectangle :)
Is the "big rect" the entire tab or is it just the grey rectangle?
Thanks so much, Viet! Some comments:
Can we make the three boxes (discrimination, lack of resources, conflict) equal width please?
Done!
Also, in the "ideal for," can we make it a true list format please? Right now, when the text bleeds to the second line, the second line doesn't align with where the first line of text starts.
Done!
The bolded text quote seems heavy -- can we unbold them and use the regular weight?
Done!
@dchen97 Can you PTAL if there is anything else that needs to be fixed?
Thanks for making those changes! Overall, it looks great! Just some small notes:
On Tue, May 21, 2019 at 3:20 PM Viet Tran Quoc Hoang < notifications@github.com> wrote:
Assigned #106 https://github.com/oppia/foundation-website/pull/106 to @dchen97 https://github.com/dchen97.
— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/oppia/foundation-website/pull/106?email_source=notifications&email_token=AC32BW44ACHMNDEDZZQ6TJLPWRKQLA5CNFSM4HM7YNK2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGORSBDTUQ#event-2357344722, or mute the thread https://github.com/notifications/unsubscribe-auth/AC32BWYKOJGBML3S7VYQLXTPWRKQLANCNFSM4HM7YNKQ .
-- Diana Chen Volunteer, Oppia Foundation
@seanlip Can you please review this PR from code perspective? I have finished Rachel and Diana latest round of comments.
@seanlip I have also dropped the numbering for the problem text. PTAL (Changes are live at https://oppia-foundation-test-server.appspot.com)
@seanlip I have just checked in the fix, albeit deceptively simple. Can you check if the tab's labels are still bugging out? It is one of those "keep refreshing the page till it breaks" bug so I would greatly appreciate a second pair of eyes :)
Just tested, and it looks great now, thanks @hoangviet1993!
However, I happened to open it first in Firefox this time, and it looks like the alignment/spacing of the buttons etc. are pretty wonky there. Could you please take a look? It looks good in Chrome, so if you can figure out why FF isn't matching Chrome and fix that, that would be great.
Just as an example, here's how one of the sections looks in Firefox:
Thanks for the great catch, @seanlip. I have made some adjustment to the HTML templates so that the page appears the same across FF, Chrome and Safari.
Thanks for the update!!
I'm seeing the same bug @seanlip mentioned with the tab width. It's fine after a hard refresh, but on the first load it's too narrow:
@rachelwchen I fixed it in the latest version. Once you clear out the test server Foundation page cache, the buggy tab label shouldn't be happening again.
Since you are here, I also think we need a different design for the student testimonial lists for mobile. What do you think about it? @rachelwchen
Gotcha, thanks for the clarifications.
Re: testimonials, what about this? Moving the picture down next to the source.
@rachelwchen Thanks Rachel! That looks much better now. Here is it now!
Thanks so much, @hoangviet1993! It looks fantastic to me overall! Just some minor formatting/rephrasing notes:
In the problems section, perhaps we can rephrase "Some problems include..." to "Some reasons why include..." to help clarify that these problems relate back to the rest of the paragraph.
On desktop, the columns in the "The Oppia Foundation is..." section is a bit far apart, which makes the text on the left look a bit small. Perhaps could these be placed closer together?
On desktop view, it personally looks a bit strange for the text to the left of the computer to be aligned at the top of the computer since it leaves so much more space at the bottom of the "See the Oppia Difference" button. Could this text and button perhaps be center-aligned to help even out the spacing?
@dchen97 I have addressed all of your comments. PTAL. Changes are live on the test server. https://oppia-foundation-test-server.appspot.com/
@dchen97 is out for a bit, so I'm going to merge this for now and we can take another pass later prior to deployment to the main server. Thanks @hoangviet1993!
PR's changes are live at: https://oppia-foundation-test-server.appspot.com Attached are some pictures:
Diana's feedbacks:
Rachel's feedbacks:
Increase the width of the big rectangle.(Comment removed)