kariefury / oppia

Automatically exported from code.google.com/p/oppia
Apache License 2.0
0 stars 0 forks source link

Code review request #626

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Branch name:
learner-view-fixes

Link to the relevant commit(s):
https://code.google.com/p/oppia/source/detail?r=4e632b006eee0f9e721f870d3d208d60
ad0d434e&name=learner-view-fixes
https://code.google.com/p/oppia/source/detail?r=0029856c4cc8012a33a48547e4673ea4
7126ec27&name=learner-view-fixes
https://code.google.com/p/oppia/source/detail?r=45cba7d6e345d396bfc03bd4e6966307
86573ced&name=learner-view-fixes

Purpose of code changes on this branch:
some UI fixes to the learner view

When reviewing my code changes, please focus on:
everything

After the review, I'll merge this branch into: develop

Original issue reported on code.google.com by wxy.xi...@gmail.com on 25 Feb 2015 at 2:11

GoogleCodeExporter commented 9 years ago
Amit, could you please take a look at this from a UI perspective (especially 
the second commit, which should finalize the scroll behavior in the learner 
view)?

In particular the autofocus has been removed. Is this what you want? In 
particular, should we autofocus when the bottom of the card is in view? This 
seems nice, but I am also unsure about consistency (it may be weird if 
sometimes we focus and sometimes we don't).

Also if you have any general requests for scrolling please make them here, 
otherwise I think this is the last of the changes we'll make to the learner 
view.

Thanks!

Original comment by s...@google.com on 25 Feb 2015 at 7:53

GoogleCodeExporter commented 9 years ago
Hi Sean,

1) The scrolling behavior is almost there, the issue is that the card should 
only scroll up to the half-way point on the screen if it's a tall card; 
otherwise, the card should scroll only until the bottom of the card is at the 
bottom of the screen. So again, just to clarify:

Case #1 (small card): If there is a card that's, for example, 20px tall on a 
100px screen, the card should scroll up until the bottom of the card is at the 
bottom of the screen. The card should *not* scroll up to the half-way point on 
the screen. This is how it was working before the new changes.

Case #2 (tall card): If there is a card that's, for example, 80px tall on a 
100px screen, the card should scroll up until the top of the card is at the 
half-way point on the screen. This is how it currently works in the new 
changes, which is great, but it breaks Case #1.

2) Regarding auto-focus: I think we should try autofocus when the bottom of the 
card is in view. Would this be hard to test? I don't think the inconsistency is 
a problem.

Thanks,
Amit

Original comment by amitdeut...@google.com on 25 Feb 2015 at 10:18

GoogleCodeExporter commented 9 years ago
SGTM. Xinyu, is this something you could do?

Thanks!

Original comment by s...@google.com on 25 Feb 2015 at 10:28

GoogleCodeExporter commented 9 years ago
Changes here, PTAL: 
https://code.google.com/p/oppia/source/detail?r=a84317854aebabc0fd60f8b50a0485f7
a610b916&name=learner-view-fixes

Original comment by wxy.xi...@gmail.com on 26 Feb 2015 at 1:44

GoogleCodeExporter commented 9 years ago
Made a few minor changes and merged to develop. Thanks a lot, this is *great*!

Original comment by amitdeut...@google.com on 26 Feb 2015 at 2:38

GoogleCodeExporter commented 9 years ago

Original comment by s...@seanlip.org on 1 Mar 2015 at 5:33