Code review request (for image click widget)

[GoogleCodeExporter]

Branch name: image-click-widget

Link to the relevant commit(s):
https://code.google.com/p/oppia/source/detail?r=890ec9d0e7d29e5f24aebf0542b3118b
f3384e69&name=image-click-widget and later commits

Purpose of code changes on this branch: Addressing minor changes 1,3,4 in issue 
531 (https://code.google.com/p/oppia/issues/detail?id=531)

When reviewing my code changes, please focus on:
Not very sure about the what would be the best option (colors, font sizes, 
positioning, etc.) for readability of the labels in the customization editor, 
any thoughts?

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

Original issue reported on code.google.com by czxcjx1...@gmail.com on 6 Feb 2015 at 9:57

[GoogleCodeExporter]

Jacob -- please could you do the code review?

Amit -- would you mind commenting on the design aspects? Note that this will 
ship in v2.

Thanks!

Original comment by s...@google.com on 6 Feb 2015 at 10:21

[GoogleCodeExporter]

Hi all, this is looking great-- well done! Here is a long list of design 
comments :) Note that some of these are probably not specific to this widget, 
feel free to separate those out and we can file them as separate issues.

-I would recommend renaming the widget from ‘image’ to ‘image region 
select’ in the interaction drop down menu.

-When there is no image selected, the image appears as a broken image icon. 
Would it be possible to replace that with help text like ‘Select an image to 
display’

-Before you upload any image, there is a sentence that says ‘The image and 
its regions’, followed by a blank white space. Once you upload an image, it 
becomes unnecessary, so can it just be removed altogether?

-When you click the upload image button, before you’ve uploaded any images, 
there is some text highlighted in yellow. I think that text needs some margins 
around it, it feels very squished against the edge of the box. The ‘Choose 
File’ (No file chose) line needs some margin to push it away from the 
yellow-highlighted text above it and from the left edge of the box. The 
‘Save’ and ‘Cancel’ buttons are also squished, they need some margins 
above and below them as well.

-Once you start drawing regions, I noticed you start at 0. Can we start at 1 
instead? I think that would be less confusing.

-Currently, when I drag an region, I expect to be able to move it. Instead, I 
end up drawing another sub-region inside of the original region. Would it be 
possible to instead have two buttons: one that puts you into draw-region mode 
which turns the mouse into a crosshair, and one ‘move’ mode that lets you 
drag regions around?

-Currently, regions are renamed and deleted in the fields below the image. 
Would it be possible to, instead, have the notion of an ‘active’ region; 
when you’re in ‘move’ mode and you click on or drag a region, that region 
becomes highlighted, and its name appears in a text box. Then you can use that 
text box to rename the region— I believe the graph input widget works 
somewhat like this. Perhaps there could be a delete button there as well. 

-When you create a rule, the top bar currently says, ‘Answer [is in the 
region]’, where [is in the region] is a disabled drop down. Would it be 
possible to get rid of the disabled drop down so that it’s a single sentence, 
‘Answer is in the region’.

-If you create a rule that refers to a region, then you rename the region, then 
the rule breaks (instead of updating the rule to the new name).

-In the player view, the image isn’t centered.

-In the player view, once you click on a region, nothing shows up as the 
user’s response. Perhaps instead of being blank, the user response can be 
something like ‘(Clicks on image)’.

-After you select a region that takes you to a new state, the image disappears. 
Would it be possible to keep the image in the conversation stream?

Thanks!

Original comment by amitdeut...@google.com on 7 Feb 2015 at 12:50

[GoogleCodeExporter]

Amit, thanks! One question:

> Currently, when I drag an region, I expect to be able to move it. Instead, I 
end up
> drawing another sub-region inside of the original region. Would it be 
possible to
> instead have two buttons: one that puts you into draw-region mode which turns 
the
> mouse into a crosshair, and one ‘move’ mode that lets you drag regions 
around?

Can you specify when exactly the cursor should be in 'move' mode and when it 
should be in 'draw-region' mode?

Also, one other question: do you think that regions should be allowed to 
overlap?

Thanks!

Original comment by s...@seanlip.org on 7 Feb 2015 at 1:50

[GoogleCodeExporter]

Currently, in the Graph widget, there are are a series of buttons on the 
left-hand side titled 'Move', 'Add Edge', 'Add Node', and 'Delete'; clicking 
one of these buttons puts the cursor into a different 'mode'. I would recommend 
having a similar mechanism here, so that one button would be called 'Move' and 
would put the cursor into a move-mode, and one button would be called 'Add 
Region', and would put the cursor into draw-mode. It would be nice if you could 
also resize regions by dragging their corners in move mode.

Re: regions overlapping, I think that's alright-- to simplify things, you can 
always follow the rule that and region that gets moved immediately moves to the 
highest z-index, so that any time you drag one region over another, the one 
that you just dragged is on 'top'. If you then start dragging the bottom 
region, it becomes on 'top'.

Original comment by amitdeut...@google.com on 7 Feb 2015 at 2:06

[GoogleCodeExporter]

Thanks for all the comments!

I've made the changes for these so far:
-I would recommend renaming the widget from ‘image’ to ‘image region 
select’ in the interaction drop down menu.
-When there is no image selected, the image appears as a broken image icon. 
Would it be possible to replace that with help text like ‘Select an image to 
display’
-Once you start drawing regions, I noticed you start at 0. Can we start at 1 
instead? I think that would be less confusing.

I'll take a look at changing the regions to be draggable/resizable and the 
response template next. Most of the remainder besides these aren't really in 
code specific to this widget, but I'll take a look at them after finishing up 
this widget.

Also, for what should show up as user's response, a few options have been 
discussed before, but I don't think there's been any decision, so would it be 
possible to get some feedback on the following options?
1) A short phrase like "(Clicks on image)"
2) The label of the region clicked
3) The image with the clicked region highlighted
4) The image with the position of the mouse click highlighted
5) Any other possibilities?

Thanks!

Original comment by czxcjx1...@gmail.com on 7 Feb 2015 at 9:46

[GoogleCodeExporter]

+Abraham

Amit -- re Zhan Xiong's last question, Abraham was also asking about this with 
regards to the map interaction. Do we have a general standard for the response 
text for these 'supplemental' interactions? I seem to recall something about 
having the response text be "(Clicks on image)" or "(Clicks on map)", and that 
this is a link that, when clicked, opens up the image/map in a modal (or 
expands it, or something). Is that still the case?

Zhan Xiong, I mildly incline against (2) since the learner won't know anything 
about what the label means. The other options sound reasonable to me, though 
(perhaps a combination of 1 and either 3 or 4), and I defer to Amit on that.

Original comment by s...@seanlip.org on 7 Feb 2015 at 9:55

[GoogleCodeExporter]

Hi Zhan Xiong, I spoke to Amit over dinner. Here's what we came up with:

- For the conversation log, do (1), but when the user hovers over that phrase, 
show a tooltip/popover that contains the image and that also has an overlaid 
translucent circle around the location of the corresponding click.
- For the main interaction itself, also show the locations of past clicks using 
similar translucent circles (similar to the map widget).

Does this sound good?

Original comment by s...@seanlip.org on 11 Feb 2015 at 5:51

• Changed title: Code review request (for image click widget)