SMILEConsortium / node-smile-server

3 stars 4 forks source link

IQManager should detect if images are available in IQSet questions and allow them to be viewed #74

Closed truedat101 closed 10 years ago

truedat101 commented 10 years ago

Right now we say "No Image Available". Should be easy now that we have the PIC and TYPE set properly.

truedat101 commented 10 years ago

Image describing issue: https://f.cloud.github.com/assets/3521208/1826970/a7a39724-721a-11e3-9e17-7fbef55b6b56.png

truedat101 commented 10 years ago

The fix here should be simple. Detect presence of PIC in the question JSON data. This will be base64 encoded image. This detection can be added in smileiqmanager.js. In the view model, add a new attribute called picdatauri and insert the data uri prefix so that the image can load the data from the base64. Here's an example of what we did in smile-student.html:

GlobalViewModel.pic(response.base64); GlobalViewModel.picdatauri('data:image/png;base64,' + response.base64);

In the HTML itself, we'd just add .

Ideally, we'd not show the image button if there is no image. This can be done easily through jquery.

truedat101 commented 10 years ago

This isn't essential for the release, so I will leave this as an open task to fix after we've release 0.5.4b.

truedat101 commented 10 years ago

Please attempt a fix @chrqls . I've outlined the details in how it should be fixed. If you continue to have network issues, you can fix these easily. Just run your server on your laptop, and use your browser connecting to localhost. You will not need wifi to test this scenario.

chrqls commented 10 years ago

I really don't know what is happening now with my network but I can do anything correctly.

I understand that for now we have the "No image available" message because there is a onclick="alert( in smile-iqmanager.html:

<tbody data-bind="foreach: iqdata">
    <tr class="iq-grid" data-bind="attr: {id: $index}">
        <td data-bind="text: $index()+1">1</td>
        <td data-bind="text: Q">Where was it?</td>
        <td data-bind="text: O1">Drawer</td>
        <td data-bind="text: O2">Shelf</td>
        <td data-bind="text: O3">TV</td>
        <td data-bind="text: O4">Floor</td>
        <td data-bind="text: A">3</td>
        <td><a class="tiny button" href="#" onclick="alert('No Images Available'); return false;"> <img src="images/photo-icon.png" /> </a></td>
    </tr>
</tbody>

Knockout give us a way to make our foreach on iqdata. So I have to find a way to get all the imageurl from this iqdata.

I think that the 2 functions we have to fix in smileiqmanager.js are function loadIQSet(evtdata, cb) and function loadSession(evtdata, cb) but impossible to continue my tests for now because of my network problems.


I wanted to see how the thing is working on smile web student to display the image. In smile-student.html, the <img /> tag looks like this:

<img data-bind="attr: { src: picdatauri}" />
chrqls commented 10 years ago

dixit @truedat101 Can you review the knockout.js information on data-bind attribute? This is an MVC pattern attribute which binds the model to the view. So if you have data for the view, it will be presented. In our case, we need to, for any legit have a src attribute, right? So typically a src attribute is a URL. In our case, we are going to provide BASE64 encoded data. See how I do this on the student html, where I upload an image. We use a special URI to bind that data. The first step here is to detect whether Question.PIC is defined. If it is defined, then present the button. Otherwise, hide the button. JQuery has nice routines to hide and unhide things.

chrqls commented 10 years ago

Until we find an another solution to keep the image button, I propose the pull request #105

truedat101 commented 10 years ago

@chrqls You need to review this to make sure that it works before commencing testing on https://github.com/SMILEConsortium/smile_teacher_android/issues/118 . It needs to work for both the IQSet Detail view and the Session detail view.

chrqls commented 10 years ago

With this change, we are able to see the picture but from the table. I have maybe a better idea, but I still work on it

truedat101 commented 10 years ago

I'll review. Please don't add jquery UI or Bootstrap. We are using Zurb Foundation, as mentioned in the issue, it can display modal images.

On Thu, Jan 23, 2014 at 3:26 AM, Charles Quelos notifications@github.comwrote:

With this change, we are able to see the picture but from the table. I have maybe a better idea, but I still work on it

— Reply to this email directly or view it on GitHubhttps://github.com/SMILEConsortium/node-smile-server/issues/74#issuecomment-33116155 .

chrqls commented 10 years ago

Let me know if the thing does not work for you, @truedat101

truedat101 commented 10 years ago

Does this work? I still get a reference error when I load iqset detail if PICURL is not defined. This kills the execution of the JS.

truedat101 commented 10 years ago

Second thing, I think it was pretty clear, see comment: https://github.com/SMILEConsortium/node-smile-server/issues/74#issuecomment-31401161 , we only want to show the button if this is a QUESTION_PIC type, right? No point in showing a button if it shows nothing. I actually preferred this when it just showed an alert "Not Implemented'" rather than having broken JS and an empty pop up.

Do you think you can take care of this the right way for both the IQSet detail and the Sessions detail?

truedat101 commented 10 years ago

This shouldn't have taken a month to fix. I can fix this in a few hours but I'm completely tied up on other work. Let me know if you don't think you can implement this the way I've recommended in the comment: https://github.com/SMILEConsortium/node-smile-server/issues/74#issuecomment-31401161

You did create this issue after all. The right behavior is to show an image and button if there is an image. We'll only have images in IQSets if we have created them from the ad hoc classroom session. We don't have a way to load them yet from the IQManager.

chrqls commented 10 years ago

We'll only have images in IQSets if we have created them from the ad hoc classroom session. We don't have a way to load them yet from the IQManager.

True. In fact, a parameter PICURL exists from iqset table, but not from session table

truedat101 commented 10 years ago

It's clear at the moment the implementation is assuming we have a url to retrieve the image. However, we don't have a URL. We have the data in the DB, so we need to load the binary data from the iqdata.

truedat101 commented 10 years ago

We need to turn off some server debugging, otherwise we print way too much data.

truedat101 commented 10 years ago

IQ Session data doesn't contain an image in the data. Need to verify.

truedat101 commented 10 years ago

Correct, the IQ Session was not saving out the picture data. My fault. I fixed that. Will commit shortly.

truedat101 commented 10 years ago

Fixed