Skip to content
This repository was archived by the owner on Jan 10, 2023. It is now read-only.

Fix modal dismissal #702

Open
rabidbit wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix modal dismissal #702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf12a39
Make modal width response
rabidbit Dec 14, 2018
b3508a1
Merge branch 'master' of https://github.com/google/tie into responsiv…
rabidbit Dec 14, 2018
7556550
Fix broken modal dismissal.
rabidbit Dec 14, 2018
e2b8800
Added E2E tests.
rabidbit Jan 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/question/components/CodeSnippetDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ tie.directive('codeSnippet', [function() {
<br>
</span>
<span ng-if="codeLines.length > MAX_NUM_LINES_IN_SNIPPET">
<a href ng-click="openCodeModal()" ng-if="!isModalOpen()">View full code</a>
<a class="tie-modal-link" href ng-click="openCodeModal()" ng-if="!isModalOpen()">View full code</a>
<span ng-if="isModalOpen()">View full code</span>
</span>
<style>
Expand Down
2 changes: 1 addition & 1 deletion client/question/components/ErrorSnippetDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ tie.directive('errorSnippet', [function() {
<div>
<p>
If you cannot figure out the problem, you can click on
<a href ng-click="openSyntaxErrorModal()" ng-if="!isModalOpen()">this link</a>
<a class="tie-modal-link" href ng-click="openSyntaxErrorModal()" ng-if="!isModalOpen()">this link</a>
<span ng-if="isModalOpen()">this link</span>
to display the error message.
</p>
Expand Down
41 changes: 38 additions & 3 deletions client/question/components/LearnerViewDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,8 @@ tie.directive('learnerView', [function() {
'FEEDBACK_CATEGORIES', 'DEFAULT_EVENT_BATCH_PERIOD_SECONDS',
'DELAY_STYLE_CHANGES', 'THEME_NAME_LIGHT', 'THEME_NAME_DARK',
'CODE_RESET_CONFIRMATION_MESSAGE', 'PRIVACY_URL', 'ABOUT_TIE_URL',
'ABOUT_TIE_LABEL', 'TERMS_OF_USE_URL',
'ABOUT_TIE_LABEL', 'TERMS_OF_USE_URL', 'FEEDBACK_MODAL_HEIGHT_OFFSET',
'FEEDBACK_MODAL_HIDE_HEIGHT_OFFSET',
function(
$scope, $interval, $timeout, $location, $window,
ConversationManagerService, QuestionDataService, LANGUAGE_PYTHON,
Expand All @@ -626,7 +627,8 @@ tie.directive('learnerView', [function() {
FEEDBACK_CATEGORIES, DEFAULT_EVENT_BATCH_PERIOD_SECONDS,
DELAY_STYLE_CHANGES, THEME_NAME_LIGHT, THEME_NAME_DARK,
CODE_RESET_CONFIRMATION_MESSAGE, PRIVACY_URL, ABOUT_TIE_URL,
ABOUT_TIE_LABEL, TERMS_OF_USE_URL) {
ABOUT_TIE_LABEL, TERMS_OF_USE_URL, FEEDBACK_MODAL_HEIGHT_OFFSET,
FEEDBACK_MODAL_HIDE_HEIGHT_OFFSET) {
$scope.PRIVACY_URL = PRIVACY_URL;
$scope.ABOUT_TIE_URL = ABOUT_TIE_URL;
$scope.ABOUT_TIE_LABEL = ABOUT_TIE_LABEL;
Expand Down Expand Up @@ -1180,13 +1182,46 @@ tie.directive('learnerView', [function() {
tasks[currentTaskIndex].getId());
};

/**
* Event handler to hide modal after transition animation ends.
*/
$scope.hideModalAfterAnimation = function(event) {
Copy link
Member

@seanlip seanlip Jan 9, 2019

Choose a reason for hiding this comment

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

I'm not sure this needs to be defined here; it duplicates code in MonospaceDisplayModalService.

$timeout(function() {
MonospaceDisplayModalService.hideModal();
}, 0);
event.target.removeEventListener("transitionend",
$scope.hideModalAfterAnimation, false);
};

/**
* Close modal by calling MonospaceDisplayModalService.closeModal.
Copy link
Member

@seanlip seanlip Jan 9, 2019

Choose a reason for hiding this comment

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

The function doesn't seem to be doing what this docstring says.

*/
$scope.closeModal = function() {
Copy link
Member

@seanlip seanlip Jan 9, 2019

Choose a reason for hiding this comment

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

The code here duplicates code in MonospaceDisplayModalDirective. In general try to strictly avoid duplicating code since that makes things harder to maintain and introduces the possibility of skew.

questionWindowDiv =
document.getElementsByClassName('tie-question-window')[0];
var modalContainerDiv = document.getElementsByClassName(
'tie-monospace-modal-container')[0];
var modalHeight = questionWindowDiv.offsetHeight;
var modalHideTopOffsetString =
'-' + (modalHeight + FEEDBACK_MODAL_HEIGHT_OFFSET +
FEEDBACK_MODAL_HIDE_HEIGHT_OFFSET).toString() + 'px';

modalContainerDiv.style.top = modalHideTopOffsetString;
modalContainerDiv.classList.remove(
'tie-feedback-modal-displayed');
modalContainerDiv.addEventListener("transitionend",
$scope.hideModalAfterAnimation, false);
};

/**
* Calls the processes necessary to start the code submission process.
*
* @param {string} code
*/
$scope.submitCode = function(code) {
MonospaceDisplayModalService.hideModal();
if (MonospaceDisplayModalService.isDisplayed()) {
$scope.closeModal();
}
SessionHistoryService.addCodeBalloon(code);

// Gather all tasks from the first one up to the current one.
Expand Down
28 changes: 18 additions & 10 deletions client/question/components/MonospaceDisplayModalDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,26 +171,34 @@ tie.directive('monospaceDisplayModal', [function() {
});

/**
* Close the modal.
* Event handler to hide modal after transition animation ends.
*/
$scope.hideModalAfterAnimation = function(event) {
Copy link
Member

@seanlip seanlip Jan 9, 2019

Choose a reason for hiding this comment

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

Consider using evt instead of event throughout (which I think we do already).

(event is a global variable in some versions of IE, so typically I avoid using it.)

$timeout(function() {
MonospaceDisplayModalService.hideModal();
}, 0);
event.target.removeEventListener("transitionend",
Copy link
Member

@seanlip seanlip Jan 9, 2019

Choose a reason for hiding this comment

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

nit: break after '(' if contents of (...) span more than one line

Here and elsewhere, use single quotes

$scope.hideModalAfterAnimation, false);
};

/**
* Close modal by calling MonospaceDisplayModalService.closeModal.
*/
$scope.closeModal = function() {
var questionWindowDiv =
document.getElementsByClassName('tie-question-window')[0];
var monospaceDisplayModalElement =
document.getElementsByTagName('monospace-display-modal')[0];

var modalContainerDiv = document.getElementsByClassName(
'tie-monospace-modal-container')[0];
var modalHeight = questionWindowDiv.offsetHeight;
var modalHideTopOffsetString =
'-' + (modalHeight + FEEDBACK_MODAL_HEIGHT_OFFSET +
FEEDBACK_MODAL_HIDE_HEIGHT_OFFSET).toString() + 'px';

monospaceDisplayModalElement.style.top = modalHideTopOffsetString;
monospaceDisplayModalElement.classList.remove(
modalContainerDiv.style.top = modalHideTopOffsetString;
modalContainerDiv.classList.remove(
'tie-feedback-modal-displayed');

$timeout(function() {
MonospaceDisplayModalService.hideModal();
}, 0);
modalContainerDiv.addEventListener("transitionend",
$scope.hideModalAfterAnimation, false);
};
}
]
Expand Down
2 changes: 1 addition & 1 deletion client/question/components/OutputSnippetDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ tie.directive('outputSnippet', [function() {
template: `
<div class="output-toggle-container">
You can click on
<a href ng-click="openOutputModal()" ng-if="!isModalOpen()"">this link</a>
<a class="tie-modal-link" href ng-click="openOutputModal()" ng-if="!isModalOpen()"">this link</a>
<span ng-if="isModalOpen()">this link</span>
to display the input, expected output and actual output.
</div>
Expand Down
81 changes: 81 additions & 0 deletions tests/e2e/question.pageObject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,37 @@ var QuestionPage = function() {
*/
var resetCodeButton = element(by.css('.protractor-test-reset-code-button'));

/**
* monospace-display-modal element.
*
* @type {webdriver.WebElement}
*/
this.monospaceDisplayModalElement =
element(by.tagName('monospace-display-modal'));

/**
* Monospace modal container element.
*
* @type {webdriver.WebElement}
*/
this.monospaceModalContainerElement =
element(by.css('.tie-monospace-modal-container'));

/**
* Modal link. To be tested with a single runCode() after resetCode().
*
* @type {webdriver.WebElement}
*/
var modalLink = element(by.css('.tie-modal-link'));

/**
* Dismiss button on modal.
*
* @type {webdriver.WebElement}
*/
var dismissModalButton =
element(by.css('.tie-monospace-modal-action-button'));

/**
* Run Code button.
*
Expand Down Expand Up @@ -119,6 +150,56 @@ var QuestionPage = function() {
await resetCodeButton.click();
};

/**
* Simulates clicking on the modal link (code, output, or error).
* To be tested with a single runCode() after resetCode().
*/
this.openModal = async function() {
await modalLink.click();
};

/**
* Simulates clicking on the dismiss modal button.
*/
this.dismissModal = async function() {
await dismissModalButton.click();
};

/**
* Gets class name for given element.
*
* @param {webdriver.WebElement} element Element to retrieve class attribute
* for.
*
* @returns {string} String containing the class names.
*/
this.getClassNames = async function(element) {
return await element.getAttribute('class');
};

/**
* Returns whether or not aria-hidden attribute is set to true.
*
* @param {webdriver.WebElement} element Element to check aria-hidden
* attribute.
*
* @returns {boolean}
*/
this.isAriaHidden = async function(element) {
return (await element.getAttribute('aria-hidden') === 'true');
};

/**
* Returns style attribute.
*
* @param {webdriver.WebElement} element Element to check top style value.
*
* @returns {string} String containing styles applied to element.
*/
this.getStyles = async function(element) {
return (await element.getAttribute('style'));
};

/**
* Simulates writing the given code string in the code editor.
*
Expand Down
29 changes: 29 additions & 0 deletions tests/e2e/question.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,35 @@ describe('Question Page', function() {
await questionPage.runCode();
});

it('should successfully open and dismiss a modal', async function() {
await questionPage.resetCode();
await questionPage.runCode();

await browser.sleep(3000);
questionPage.openModal();
await browser.sleep(2000);

expect(await questionPage.getClassNames(element(by.tagName(
'monospace-display-modal')))).not.toContain('ng-hide');
expect(await questionPage.getClassNames(
questionPage.monospaceModalContainerElement)).toContain(
'tie-feedback-modal-displayed');
expect(await questionPage.isAriaHidden(
questionPage.monospaceDisplayModalElement)).toBe(false);
expect(await questionPage.getStyles(
questionPage.monospaceModalContainerElement)).not.toContain('top: ');

questionPage.dismissModal();
await browser.sleep(2000);

expect(await questionPage.getClassNames(
questionPage.monospaceDisplayModalElement)).toContain('ng-hide');
expect(await questionPage.isAriaHidden(
questionPage.monospaceDisplayModalElement)).toBe(true);
expect(await questionPage.getStyles(
questionPage.monospaceModalContainerElement)).toContain('top: ');
});

it('should display a feedback text paragraph after a run', async function() {
await questionPage.resetCode();
await questionPage.runCode();
Expand Down