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

Fixed : issue/#2100 #182

Merged
merged 3 commits into from
Jun 5, 2018
Merged

Conversation

NayanKhedkar
Copy link
Member

set visited to current stage item when moving from small/medium screen size to large.

set visited to current stage item when moving from small/medium screen size to large.
@NayanKhedkar
Copy link
Member Author

Issue : adaptlearning/adapt_framework#2100

if (!this.getCurrentItem(stage)._isVisited) {
this.setStage(stage, true);
}
}
this.calculateWidths();
this.evaluateNavigation();
Copy link
Member

@oliverfoster oliverfoster May 29, 2018

Choose a reason for hiding this comment

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

            var wasDesktop = this.model.get('_isDesktop');
            this.setDeviceSize();
            var isDesktop = this.model.get('_isDesktop');
            if (wasDesktop !== isDesktop) this.replaceInstructions();
            this.calculateWidths();
            this.evaluateNavigation();
            /*set current stage item to visited when moving from small/medium screen size to large*/
            if (!isDesktop) return;
            var stage = this.model.get('_stage') || 0;
            if (this.getCurrentItem(stage)._isVisited) return;
            this.setStage(stage, true);

You don't need to define isDesktop at the top if you're going to redefine its value immediately afterward.
Reducing that double nesting of the two if blocks can be achieved by moving it to a separate function or by moving it to the end of the block and returning early. Really a separate function would work, something like setCurrentItemVisited() ?
Then it would be:

            var wasDesktop = this.model.get('_isDesktop');
            this.setDeviceSize();
            var isDesktop = this.model.get('_isDesktop');
            if (wasDesktop !== isDesktop) this.replaceInstructions();
            this.calculateWidths();
            this.evaluateNavigation();
            /*set current stage item to visited when moving from small/medium screen size to large*/
            if (isDesktop) this.setCurrentItemVisited();

setCurrentItemVisited: function() {
  var stage = this.model.get('_stage') || 0;
  if (this.getCurrentItem(stage)._isVisited) return;
  this.setStage(stage, true);
},

this.calculateWidths();
this.evaluateNavigation();
/*set current stage item to visited when moving from small/medium screen size to large*/
Copy link
Contributor

Choose a reason for hiding this comment

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

could you modify this to have a link to adaptlearning/adapt_framework#2100 i.e.

/**
 * need to set current stage item to visited when moving from small/medium screen size to large
 * see https://github.com/adaptlearning/adapt_framework/issues/2100
 */

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@moloko moloko merged commit a5f4eb2 into adaptlearning:master Jun 5, 2018
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.

4 participants