directorlive / oppia

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

Code review request #608

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Branch name: end-conversation

Link to the relevant commit(s):
(1) 
https://code.google.com/p/oppia/source/detail?r=cfc111e02560673382308a91dc75ee04
8da40188&name=end-conversation
(2) 
https://code.google.com/p/oppia/source/detail?name=end-conversation&r=f11c54f012
206dd079720f386692883f027f7c16

Purpose of code changes on this branch: Add an EndConversation 'interaction' 
that offers links back to the gallery and to recommended explorations. This 
replaces the original idea of 'adventures'.

When reviewing my code changes, please focus on:
- Amit: UI
- Jacob: code
- Xinyu: changes made to files relating to the exploration history tab (other 
comments are also welcome)

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

Original issue reported on code.google.com by s...@seanlip.org on 17 Feb 2015 at 8:19

GoogleCodeExporter commented 9 years ago
The changes relating to the history tab look good.
I have a few comments from a user perspective:
- The interaction is not really a 'input' (or at least, it is significantly 
different from the other inputs in that it ends the conversation), so perhaps 
it can be moved out of the 'basic input' dropdown?
- I found it a bit strange that I am required to have an 'END' state even if 
some of my states have end interactions (i.e. the warnings prevent me from 
saving the exploration). Can the checking for readability of states take into 
account end interactions?
- (Relating to the above, how does the end conversation interaction work with 
the current 'END' state? Is it meant as a replacement for END, or as only as a 
secondary way authors can end the exploration?)
- If a state currently only points to 'END' (in the state graph), and I change 
that state's interaction to End Conversation, the state graph is not updated to 
show that the state no longer points to 'END'
- Perhaps there can be an indicator that a state is an end state (rather than a 
state leading to nowhere) in the state graph

Thanks!

Original comment by wxy.xi...@gmail.com on 19 Feb 2015 at 3:38

GoogleCodeExporter commented 9 years ago
Thanks a lot, Xinyu! I think I have addressed all these comments in 'develop'; 
PTAL if you have time.

Regarding your third point, the end conversation interaction is meant as a 
secondary way for authors to end the exploration, but I am hoping that in the 
future we can get rid of the END state and make the interaction the only way to 
do so. The current END state is a bit awkward with its congratulatory text that 
cannot be customized.

Original comment by s...@google.com on 20 Feb 2015 at 2:55

GoogleCodeExporter commented 9 years ago
Thanks for clarifying about the role of end conversation. I agree that the 
interaction seems to be a better way to end the exploration since it can be 
customized to the needs of different explorations.

I've taken a brief look at the changes you made. They look great, and I also 
like how you've arranged all the interactions into clearer categories. Thanks!

Original comment by wxy.xi...@gmail.com on 20 Feb 2015 at 5:02

GoogleCodeExporter commented 9 years ago

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