New feature for lti module. Open in new window. #1185

valera-rozuvan · 2013-10-01T14:03:25Z

Add ability to display LTI module in a new window. Iframe will be still supported. The flag which determines what functionality is used can be set from the Edit dialog in Studio per individual LTI.

Python tests, acceptance tests, and Jasmine tests were updated.

Reviewers:
@frrrances
@Lyla-Fischer
@singingwolfboy

valera-rozuvan · 2013-10-02T10:23:37Z

@polesye I tried going with the Underscore templates, but I was not successful. It will take a lot of time for me to implement such a solution. In the end I went with jQuery and modifying our element on the fly.

Maybe this is not optimal, but it does reduce the number of fixtures to 1.

^_^

frrrances · 2013-10-02T20:03:37Z

lms/templates/lti.html

I'm worried this wording is confusing to students and the plain link style is easy to miss in a vertical, so I think we need a bit more here. I would change it to:

[LTI Display Name] (External resource) <a href="#" class='link_lti_new_window'> ${_('View resource in a new window')}</a> 

where [LTI Display Name] is the text that the Studio user puts into the display name input.

In addition, it needs some styling to make it a bigger element in the vertical with a clear button, similar to discussions:

@valera-rozuvan please add this styling

auraz · 2013-10-04T13:29:50Z

common/lib/xmodule/xmodule/js/src/lti/lti.js

This is checked in template, do not check it twice.

auraz · 2013-10-04T13:34:05Z

@valera-rozuvan Please add styling pointed by Frances

singingwolfboy · 2013-10-04T13:58:08Z

lms/templates/lti.html

Shouldn't "External resource" be translated?

valera-rozuvan · 2013-10-09T08:37:14Z

@singingwolfboy I have addressed your template translation comments.

valera-rozuvan · 2013-10-09T08:37:51Z

@auraz I have removed check for empty and default URL in JavaScript (lti module, not Jasmine test).

valera-rozuvan · 2013-10-09T08:40:47Z

@frrrances I am having trouble identifying the styles I need in order to make the link stand out more. My HTML code is:

<section class="xmodule_display xmodule_LTIModule" data-type="LTI">
    <div id="i4x-private-01-lti-4757d22a9c974afc8c39e777753f247e" class="lti" data-open_in_a_new_page="true">
        <p>
            <span class="description">
                Book 1 (External resource)
            </span>
            <a href="#" class="link_lti_new_window">
                <i class="icon-external-link"></i>
                View resource in a new window
            </a>
        </p>
    </div>
</section>

Can you please suggest?

valera-rozuvan · 2013-10-09T08:49:05Z

@auraz I have actually added back the check for URL. If it is empty or default - then we must make sure that JS part does not try to make a form submit. I know that the check is in the template. But that check only adds or removes iframe or link.

frrrances · 2013-10-09T20:57:34Z

@valera-rozuvan I made pushed some changes to the sass and adjusted the html a little for the LTI in an external window. Let me know if that helps or if you need me to make adjustments. thanks!

valera-rozuvan · 2013-10-10T07:49:23Z

@frrrances I like your changes. I have just tweaked them a bit to make the link and the text on the same level, and more in the center.

@Lyla-Fischer @singingwolfboy @frrrances Am I good to merge?

frrrances · 2013-10-10T13:51:16Z

looks good to me! 👍

Add ability to display LTI module in a new window. Iframe will be still supported. Fix python test Update acceptance tests Fix in acceptance tests, they are stable now Fix Acceptance tests. Adding Jasmine tests for LTI. Fixing Jasmine tests. Adding the rest of Jasmine tests.

Removed JS check for empty url and default url. In template added i18n support for several strings.

Made it so that the link and the text are on the same level, and in the center of the block.

New feature for lti module. Open in new window.

…anslations-for-multilingual-support Fix translation for cert name in dashboard openedx#1185

…ail-templates-for-multilingual-support Fix email templates for multilingual support openedx#1185

ghost assigned singingwolfboy Oct 1, 2013

frrrances reviewed Oct 2, 2013
View reviewed changes

ghost assigned auraz Oct 4, 2013

auraz reviewed Oct 4, 2013
View reviewed changes

common/lib/xmodule/xmodule/js/src/lti/lti.js

Copy link

Contributor

auraz Oct 4, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is checked in template, do not check it twice.

ghost assigned nedbat Oct 4, 2013

singingwolfboy reviewed Oct 4, 2013
View reviewed changes

lms/templates/lti.html Outdated

Copy link

Contributor

singingwolfboy Oct 4, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't "External resource" be translated?

auraz and others added 12 commits October 14, 2013 13:58

Updated Jasmine tests as per Anton's and Alex's suggestions.

67463da

Oops. Forgot to delete one more template.

a950bb7

Updating LTI template.

1fe33a0

Add display_name to lti template

f9b64ca

Adding fix for failing Python unit test.

134eab8

Addressing comments by auraz and singingwolfboy.

3c6591e

Removed JS check for empty url and default url. In template added i18n support for several strings.

Fixing template bug.

e2fb2ae

Added back check in JS for empty or default URL.

068d0ce

styling for LTI in a new window block

2877345

Minor styling tweaks. Added Changelog entry.

06a85e6

Made it so that the link and the text are on the same level, and in the center of the block.

Fix attribute target.

e76e862

valera-rozuvan added a commit that referenced this pull request Oct 15, 2013

Merge pull request #1185 from edx/alex/lti_new_window

5592511

New feature for lti module. Open in new window.

valera-rozuvan merged commit 5592511 into master Oct 15, 2013

valera-rozuvan deleted the alex/lti_new_window branch October 15, 2013 12:07

jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Sep 16, 2016

Fix translation for cert name in dashboard openedx#1185

ca88308

jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Sep 16, 2016

Merge pull request openedx#1198 from yokose-ks/develop/dogwood/fix-tr…

f200613

…anslations-for-multilingual-support Fix translation for cert name in dashboard openedx#1185

jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Sep 27, 2016

Fix email templates for multilingual support openedx#1185

860ed59

jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Sep 27, 2016

Merge pull request openedx#1208 from yokose-ks/develop/dogwood/fix-em…

d8fe52f

…ail-templates-for-multilingual-support Fix email templates for multilingual support openedx#1185

New feature for lti module. Open in new window. #1185

New feature for lti module. Open in new window. #1185

Uh oh!

Conversation

valera-rozuvan commented Oct 1, 2013

Uh oh!

valera-rozuvan commented Oct 2, 2013

Uh oh!

frrrances Oct 2, 2013

Choose a reason for hiding this comment

Uh oh!

auraz Oct 4, 2013

Choose a reason for hiding this comment

Uh oh!

auraz Oct 4, 2013

Choose a reason for hiding this comment

Uh oh!

auraz commented Oct 4, 2013

Uh oh!

singingwolfboy Oct 4, 2013

Choose a reason for hiding this comment

Uh oh!

valera-rozuvan commented Oct 9, 2013

Uh oh!

valera-rozuvan commented Oct 9, 2013

Uh oh!

valera-rozuvan commented Oct 9, 2013

Uh oh!

valera-rozuvan commented Oct 9, 2013

Uh oh!

frrrances commented Oct 9, 2013

Uh oh!

valera-rozuvan commented Oct 10, 2013

Uh oh!

frrrances commented Oct 10, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants