Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rename scenario for Internet Explorer #2321

Merged
merged 1 commit into from
Oct 4, 2017

Conversation

caseycesari
Copy link
Member

Overview

Using _.curry appeared to call the validate function before it needed to
be called, which resulted in a JS error and prevented the modal from
being displayed. Keeping the correct scope for the validate function is
slightly reworked here so that _.curry does not need to be used, which
lets things work as expected in IE.

Connects to #2315

Demo

mmw2

Testing Instructions

  • Create a new project and add a scenario.
  • Try to rename the scenario to a valid name. It should work.
  • Try to rename the scenario to a name that already exists. It should show an error message.

@arottersman
Copy link

Heads up, a few of the tests broke

not ok 112 Firefox 56.0 - Modeling Models ScenarioCollection #validateNewScenarioName returns null if valid rename
    ---
        message: >
            newName.trim is not a function
        stack: >
            validateNewScenarioName@http://localhost:7357/7641/js/test.bundle.js:18920:30
            [136]</</</</</<@http://localhost:7357/7641/js/test.bundle.js:21707:45
            Runnable.prototype.run@http://localhost:7357/testem/mocha.js:4212:24
            Runner.prototype.runTest@http://localhost:7357/testem/mocha.js:4584:5
            next/<@http://localhost:7357/testem/mocha.js:4630:7
            next@http://localhost:7357/testem/mocha.js:4510:14
            next/<@http://localhost:7357/testem/mocha.js:4519:7
            next@http://localhost:7357/testem/mocha.js:4463:23
            Runner.prototype.hook/<@http://localhost:7357/testem/mocha.js:4487:5
            timeslice@http://localhost:7357/testem/mocha.js:5480:5
            
    ...
not ok 113 Firefox 56.0 - Modeling Models ScenarioCollection #validateNewScenarioName ignores case when comparing the new name with existing names
    ---
        message: >
            newName.trim is not a function
        stack: >
            validateNewScenarioName@http://localhost:7357/7641/js/test.bundle.js:18920:30
            [136]</</</</</<@http://localhost:7357/7641/js/test.bundle.js:21715:45
            Runnable.prototype.run@http://localhost:7357/testem/mocha.js:4212:24
            Runner.prototype.runTest@http://localhost:7357/testem/mocha.js:4584:5
            next/<@http://localhost:7357/testem/mocha.js:4630:7
            next@http://localhost:7357/testem/mocha.js:4510:14
            next/<@http://localhost:7357/testem/mocha.js:4519:7
            next@http://localhost:7357/testem/mocha.js:4463:23
            Runner.prototype.hook/<@http://localhost:7357/testem/mocha.js:4487:5
            timeslice@http://localhost:7357/testem/mocha.js:5480:5
            
    ...
not ok 114 Firefox 56.0 - Modeling Models ScenarioCollection #validateNewScenarioName will not show error when leaving name as is
    ---
        message: >
            newName.trim is not a function
        stack: >
            validateNewScenarioName@http://localhost:7357/7641/js/test.bundle.js:18920:30
            [136]</</</</</<@http://localhost:7357/7641/js/test.bundle.js:21723:45
            Runnable.prototype.run@http://localhost:7357/testem/mocha.js:4212:24
            Runner.prototype.runTest@http://localhost:7357/testem/mocha.js:4584:5
            next/<@http://localhost:7357/testem/mocha.js:4630:7
            next@http://localhost:7357/testem/mocha.js:4510:14
            next/<@http://localhost:7357/testem/mocha.js:4519:7
            next@http://localhost:7357/testem/mocha.js:4463:23
            Runner.prototype.hook/<@http://localhost:7357/testem/mocha.js:4487:5
            timeslice@http://localhost:7357/testem/mocha.js:5480:5
            

Using _.curry appeared to call the validate function before it needed to
be called, which resulted in a JS error and prevented the modal from
being displayed. Keeping the correct scope for the validate function is
slightly reworked here so that _.curry does not need to be used, which
lets things work as expected in IE.

Also, update tests for new function signature.

Refs #2315
@caseycesari
Copy link
Member Author

Thanks. Just pushed an amended commit to address the test failures.

Copy link

@arottersman arottersman left a comment

Choose a reason for hiding this comment

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

+1, working as expected on IE11 and all other supported browsers.

@@ -457,14 +457,13 @@ var ScenarioDropDownMenuOptionsView = Marionette.ItemView.extend({
renameScenario: function() {
var self = this,
collection = self.model.collection,
validate = _.bind(collection.validateNewScenarioName, collection),
curriedValidationFunction = _.curry(validate)(this.model),
validate = _.bind(collection.validateNewScenarioName, self.model),

Choose a reason for hiding this comment

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

Don't think we should change it, but could we have also moved this method onto the ScenarioModel and avoided the binding?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, totally. I think that would work as well.

@caseycesari
Copy link
Member Author

Thanks for the review.

@caseycesari caseycesari merged commit 8edf5e3 into release/1.20.0 Oct 4, 2017
@caseycesari caseycesari deleted the cpc/fix-ie-scenario-rename branch October 4, 2017 18:21
@rajadain rajadain mentioned this pull request Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants