lduarte1991 / hxat

Contains the currently-in-development project by HarvardX to bring the annotation tool currently living in the edX platform to a more accessible LTI implementation.
13 stars 7 forks source link

Annotation Modal CSS Change #55

Closed arthurian closed 8 years ago

arthurian commented 8 years ago

This PR addresses two issues with the annotation modal that we've been having:

  1. https://github.com/Harvard-ATG/annotationsx/issues/52
  2. https://github.com/Harvard-ATG/annotationsx/issues/51

I modified the CSS/layout of the annotationModal to change how the child elements are positioned to correspond to this model:

annotationmodal

I tested it in Firefox, Chrome, and Safari with both Text and Image annotations. It seems to resolves both issues.

@lduarte1991 Any thoughts on the changes? Does this break anything for you or is there anything I forgot to take into account?

lduarte1991 commented 8 years ago

That's definitely a fantastic PR. I'll play around a bit more and just run through my checklist of items to make sure nothing is particularly broken. Some quick things I noticed while playing with it:

1) In the modal view, there appears to be a white section to the far right that then turns the a grayish color after about a window's height of scrolling. Did you shift the dashboard over a bit? Maybe for scrollbar's sake? If so can we add a bit of padding for those of us that like to keep scrollbars off?

2) Clicking an annotation to take you into modal view keeps you scrolled down however far you are, which is not ideal as it starts the modal view a portion of the way down. I'm assuming that the issue then becomes that when you hit back you're not where you left off?

lduarte1991 commented 8 years ago

There also seems to be some issues when deleting (i.e. it deletes the annotation but doesn't remove it from the dashboard). I can create and edit just fine. It seems like if I delete an annotation I just created it works. If I delete something I just edited it works, but old annotations that I haven't recently (as in the last time I refreshed this page) don't seem to delete from the dashboard and I'm not noticing any errors in the console. Can you replicate any of this? If not I may have to dive down and see what I might have caused.

arthurian commented 8 years ago

@lduarte1991 I think the first one might be due to the padding I added to the annotationModal to account for the scrollbar, so let me rework that. For the second one, do you mean it starts you scrolled down the list rather than starting with the parentAnnotation? If so, that's definitely not what was intended. It must be remembering the scroll location when you hit the back button, so I'll look at that.

For deleting, I haven't noticed that issue. When I click the "delete" button the annotation, it removes the annotation from the dashboard as expected. Same thing with replies. When I delete the reply, it removes it from the replies list. Hmm, I'm wondering how I can replicate that. Just to be sure, you've cleared your cache so you're getting the latest JS/CSS right? I only mention it because I've been burnt more than once by the browser caching the JS/CSS when I didn't expect it.

I'm going to go work on the first two issues that you mentioned and come back with (hopefully) some improvements that fixes those. I'll try some more things to see if I can replicate the deleting issue. Thanks for all the feedback btw!

lduarte1991 commented 8 years ago

Actually, I should have specified. This is an issue when you delete from the Annotator viewer pop up. not from within the dashboard. I'll double check the cache. I've been burn many-a-time myself so I understand haha.

arthurian commented 8 years ago

@lduarte1991 I just committed changes that should resolve the first two issues that you observed.

1) The white section that you saw on the right-hand side should be gone regardless of whether the scrollbar is there or not. Also, instead of having the JS hide/show the scrollbar on the annotationSection programmatically, the browser should be able to do that automatically now, which should simplify the code slightly. Better to let the browser manage that if possible, I think.

2) Clicking into an annotation modal should automatically bring you to the top of the dashboard so you're viewing the parentAnnotation. It should also remember where you left off in the annotation list, so when you leave the modal, you're returned to the same scroll position.

3) I haven't been able to replicate the issue you described for deleting an annotation in an Annotator viewer pop up from within the dashboard. When I click the "x" in the Annotator viewer pop up to delete the annotation, it removes the annotation from the dashboard as expected. I've tried creating it, refreshing the page, and then attempting to delete it. Create it, then delete it without a refresh, etc. Not sure what other sequence I should try.

Let me know if my latest commits seem to fix the first two issues that you noticed or not.

lduarte1991 commented 8 years ago

1 and 2 look great. Yeah I'm still getting issues with number 3...maybe it has to do with annotations made before I switched over to the new branch? Unsure. I'll keep taking a look. Once I have an idea what's going on I can merge it.

lduarte1991 commented 8 years ago

@arthurian Actually can you do me a huge favor: In AnnotationStoreController.js can you replace the removeAnnotationFromMasterList function to be the following:

AnnotatorEndpointController.prototype.removeAnnotationFromMasterList = function(annotation) {
    var found = -1;
    this.annotationsMasterList.forEach(function(ann, index) {
        if (annotation.id === ann.id) {
            found = index;
        };
    });
    if (found > -1) {
        this.annotationsMasterList.splice(found, 1);
        return false;
    };
    return true;
};

Apparently Annotator sometimes messes up the annotation pass back and doesn't have the highlights attribute so it could not find it the way it was originally.

arthurian commented 8 years ago

@lduarte1991 Sure, I'll go ahead and make that change now.

arthurian commented 8 years ago

@lduarte1991 I updated removeAnnotationFromMasterList() (be4cbda), so give that a try.

lduarte1991 commented 8 years ago

Yup! That fixed it. Thanks :+1: