-
Notifications
You must be signed in to change notification settings - Fork 39
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
ItemsComponentModel #179
ItemsComponentModel #179
Conversation
get index from item rather then introduce a new method in model
calcualte width only once use transform rather than margin for better performance
requires these changes in the Framework |
js/narrativeView.js
Outdated
}, | ||
|
||
preRender: function() { | ||
this.listenTo(Adapt, 'device:changed', this.reRender, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind changing this to the 'object style' while you're here? i.e.
this.listenTo(Adapt, {
'device:changed': this.reRender,
'device:resize': this.resizeControl,
'device:changed': this.reRender
});
js/narrativeView.js
Outdated
}, | ||
|
||
prepareHotgraphicModel: function() { | ||
const model = this.model; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid const
and let
stop the minification process from running (see adaptlearning/adapt_framework#1966) so have to be dropped :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn, I really like const and let. Will update!
js/narrativeView.js
Outdated
prepareHotgraphicModel: function() { | ||
const model = this.model; | ||
model.resetActiveItems(); | ||
model.set('_isPopupOpen', false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you use the object style of model.set
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a couple of minor syntactical things which you can ignore if you want!
js/narrativeView.js
Outdated
calculateWidths: function() { | ||
var itemCount = this.model.get('_items').length; | ||
this.model.set('_totalWidth', 100 * itemCount); | ||
this.model.set('_itemWidth', 100 / itemCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opportunity to set via single statement eg:
this.model.set({
'_totalWidth': 100 * itemCount,
'_itemWidth': 100 / itemCount
});
js/narrativeView.js
Outdated
}, | ||
|
||
closeNotify: function() { | ||
this.evaluateCompletion() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing semi-colon
js/narrativeView.js
Outdated
} | ||
}, | ||
|
||
prefixHelper: function(elm, prop, val) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefixing is no longer required for the transform property
https://caniuse.com/#search=transform
js/narrativeView.js
Outdated
this.$('.narrative-strapline-title').a11y_focus(); | ||
} | ||
} | ||
}, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return early?
if (this._isInitial) return;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a property of the view I think it would benefit from being declared (and set) formally at the top of the code e.g.
var NarrativeView = ComponentView.extend({
_isInitial: true,
events: {
I would also be tempted to give it a more descriptive name (or add an explanatory comment to the declaration) as its purpose isn't 100% clear
.narrative-strapline-header-inner { | ||
transition: transform 400ms ease-in-out; | ||
} | ||
} | ||
} | ||
|
||
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
js/narrativeView.js
Outdated
this.setCompletionStatus(); | ||
} | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove some of the nesting?
inview: function(event, visible, visiblePartX, visiblePartY) {
if (!visible) return;
if (visiblePartY === 'top') {
this._isVisibleTop = true;
} else if (visiblePartY === 'bottom') {
this._isVisibleBottom = true;
} else {
this._isVisibleTop = true;
this._isVisibleBottom = true;
}
var wasAllInview = (this._isVisibleTop && this._isVisibleBottom);
if (!wasAllInview) return;
this.$('.component-inner').off('inview');
this.setCompletionStatus();
},
js/narrativeView.js
Outdated
this.$('.narrative-control-right').removeClass('narrative-hidden'); | ||
} | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove some of the nesting?
evaluateNavigation: function() {
var active = this.model.getActiveItem();
if (!active) return;
var currentStage = active.get('_index');
var itemCount = this.model.get('_items').length;
if (currentStage == 0) {
this.$('.narrative-controls').addClass('narrative-hidden');
if (itemCount > 1) {
this.$('.narrative-control-right').removeClass('narrative-hidden');
}
return;
}
this.$('.narrative-control-left').removeClass('narrative-hidden');
var isAtEnd = (currentStage == itemCount - 1);
this.$('.narrative-control-right').toggleClass('narrative-hidden', isAtEnd);
},
Is there meant to be a visited state? |
@tomgreenfield done |
.narrative-item-title appears in narrative.less, but I cannot find it in narrative.hbs. Is it supposed to be .narrative-content-title? |
js/narrativeView.js
Outdated
var currentStage = active.get('_index'); | ||
var itemCount = this.model.get('_children').length; | ||
|
||
if (currentStage == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use ===
here
also just to check whether this PR will require a fix for adaptlearning/adapt_framework#2100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Have done a first pass on the code.
js/narrativeView.js
Outdated
|
||
postRender: function() { | ||
this.renderState(); | ||
this.$('.narrative-slider').imageready(_.bind(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can become
this.$('.narrative-slider').imageready(this.setReadyStatus.bind(this));
js/narrativeView.js
Outdated
|
||
setupNarrative: function() { | ||
this.setDeviceSize(); | ||
if (!this.model.has('_children') || !this.model.get('_children').length) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we use _children
on line 89, it would make sense to cache here:
var items = this.model.get('_children');
if (!items || !items.length) return;
js/narrativeView.js
Outdated
moveSliderToIndex: function(itemIndex, shouldAnimate) { | ||
var invert = (Adapt.config.get('_defaultDirection') === 'ltr') ? 1 : -1; | ||
|
||
var offset = 100 / this.model.get('_children').length * itemIndex * -1 * invert; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_itemWidth
is set on the model at line 105; is it possible to use this?
js/narrativeView.js
Outdated
var index = this.model.getActiveItem().get('_index'); | ||
if (this.model.get('_isDesktop')) { | ||
if (!this._isInitial) { | ||
this.$('.narrative-content-item').eq(index).a11y_focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please switch from eq()
to using data-index
attributes.
js/narrativeView.js
Outdated
item.toggleVisited(true); | ||
} | ||
|
||
this.$('.narrative-progress:visible').removeClass('selected').eq(index).addClass('selected'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please filter()
by data-index
instead of eq()
.
js/narrativeView.js
Outdated
|
||
this.$('.narrative-progress:visible').removeClass('selected').eq(index).addClass('selected'); | ||
this.$('.narrative-slider-graphic').children('.controls').a11y_cntrl_enabled(false); | ||
this.$('.narrative-slider-graphic').eq(index).children('.controls').a11y_cntrl_enabled(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please cache this.$('.narrative-slider-graphic')
and switch to using data-index
.
js/narrativeView.js
Outdated
this.$('.narrative-slider-graphic').children('.controls').a11y_cntrl_enabled(false); | ||
this.$('.narrative-slider-graphic').eq(index).children('.controls').a11y_cntrl_enabled(true); | ||
this.$('.narrative-content-item').addClass('narrative-hidden').a11y_on(false).eq(index).removeClass('narrative-hidden').a11y_on(true); | ||
this.$('.narrative-strapline-title').a11y_cntrl_enabled(false).eq(index).a11y_cntrl_enabled(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more eq()
s to be replaced here.
js/narrativeView.js
Outdated
this.setDeviceSize(); | ||
if (!this.model.has('_children') || !this.model.get('_children').length) return; | ||
|
||
this.model.set('_active', true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query: is this definitely suitable as a model attribute?
|
||
evaluateNavigation: function() { | ||
var active = this.model.getActiveItem(); | ||
if (!active) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check needed here?
Since resizeControl()
already checks for an active item, should we just move the call to evaluateNavigation()
to sit after the check in there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to add a check for #2100 9b89d5a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just two suggestions, looks good
js/narrativeView.js
Outdated
if (Adapt.config.get('_disableAnimation')) { | ||
this.onTransitionEnd(); | ||
} else { | ||
$sliderElm.on('transitionend', this.onTransitionEnd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you changed this to
$sliderElm.one('transitionend', this.onTransitionEnd.bind(this));
then L32 and L185-L187 can all be deleted
js/narrativeView.js
Outdated
|
||
setupEventListeners: function() { | ||
if (this.model.get('_setCompletionOn') === 'inview') { | ||
this.$('.component-widget').on('inview', _.bind(this.inview, this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be changed to:
this.$('.component-widget').on('inview', this.inview.bind(this));
FYI tested in FF and IE11, checked that:
all work OK |
js/narrativeView.js
Outdated
|
||
if ($(event.currentTarget).hasClass('narrative-control-right')) { | ||
stage++; | ||
this.model.setActiveItem(stage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could just do this.model.setActiveItem(++stage);
making line above redundant (and similar for L271-272)
js/narrativeView.js
Outdated
stage--; | ||
this.model.setActiveItem(stage); | ||
} | ||
stage = (stage + numberOfItems) % numberOfItems; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line needs dealing with... I think it can just go now??
IIRC from the time I tried to figure out what this did it 'normalises' the value of stag
e i.e. if it somehow got set to -2 then it would 'translate' that to 3. Talk about an excellent example of a bit code that could really have used a comment..!
js/narrativeView.js
Outdated
moveSliderToIndex: function(itemIndex, shouldAnimate) { | ||
var invert = (Adapt.config.get('_defaultDirection') === 'ltr') ? 1 : -1; | ||
|
||
var offset = this.model.get('_itemWidth') * itemIndex * -1 * invert; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the * -1
here is redundant if you flip the values from the line above? or indeed only multiple by -1 if direction is 'ltr'
, losing the need for invert
completely?
(caveat: I'm really bad at maths so do check this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is this in a more condensed way:
var offset;
if (Adapt.config.get('_defaultDirection') === 'ltr') {
offset = offset = this.model.get('_itemWidth') * itemIndex * -1;
} else {
offset = this.model.get('_itemWidth') * itemIndex;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moloko, are you suggesting something like the below?
var offset = this.model.get('_itemWidth') * itemIndex;
if (Adapt.config.get('_defaultDirection') === 'ltr') {
offset *= -1;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomgreenfield yes - I think that would have exactly the same effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although I think it should be "multiply by -1 if direction is ltr"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lc-thomasberger as per @tomgreenfield 's code above - should work the same and avoids the need for else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry last few things
js/narrativeView.js
Outdated
@@ -265,13 +264,11 @@ define([ | |||
var numberOfItems = this.model.get('_children').length; | |||
|
|||
if ($(event.currentTarget).hasClass('narrative-control-right')) { | |||
stage++; | |||
this.model.setActiveItem(stage); | |||
this.model.setActiveItem(stage+=1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you change to this.model.setActiveItem(++stage);
? the pre-increment operator is pretty standard JS
js/narrativeView.js
Outdated
} else if ($(event.currentTarget).hasClass('narrative-control-left')) { | ||
stage--; | ||
this.model.setActiveItem(stage); | ||
this.model.setActiveItem(stage-=1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you change to this.model.setActiveItem(--stage);
? the pre-decrement operator is pretty standard JS
js/narrativeView.js
Outdated
} | ||
stage = (stage + numberOfItems) % numberOfItems; | ||
stage = this.clamp(stage, 0, numberOfItems); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really think you can delete this line as stage
is a local variable and this is the last line of the function
js/narrativeView.js
Outdated
_.bindAll(this, 'onTransitionEnd'); | ||
}, | ||
|
||
clamp: function(number, min, max) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function isn't needed
js/narrativeView.js
Outdated
|
||
onProgressClicked: function(event) { | ||
event && event.preventDefault(); | ||
var clickedIndex = $(event.target).index(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please switch to .data('index')
.
js/narrativeView.js
Outdated
ComponentView.prototype.remove.apply(this, arguments); | ||
}, | ||
|
||
getSlideDirection: function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused function?
js/narrativeView.js
Outdated
this.$('.narrative-control-left').removeClass('narrative-hidden'); | ||
|
||
var idAtEnd = (currentStage === itemCount - 1); | ||
this.$('.narrative-control-right').toggleClass('narrative-hidden', idAtEnd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a variable for both limits to get rid of the nested if statements?
var isAtStart = currentStage === 0;
var isAtEnd = currentStage === itemCount - 1;
this.$('.narrative-control-left').toggleClass('narrative-hidden', isAtStart);
this.$('.narrative-control-right').toggleClass('narrative-hidden', isAtEnd);
js/narrativeView.js
Outdated
this.model.set('_isDesktop', true); | ||
} else { | ||
this.$el.addClass('mobile').removeClass('desktop'); | ||
this.model.set('_isDesktop', false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can ditch the ambiguous desktop
and mobile
classes if you wanted and style using the existing .size-large
class.
This function could then become:
this.model.set('_isDesktop', Adapt.device.screenSize === 'large');
js/narrativeView.js
Outdated
this.$('.narrative-strapline-title').a11y_focus(); | ||
} | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return early?
if (this._isInitial) return;
js/narrativeView.js
Outdated
var items = this.model.get('_children'); | ||
if (!items || !items.length) return; | ||
|
||
this._active = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as with _isInitial
I think _active
should be declared properly at the top of the file together with a comment. Might be worth checking it's even still needed and not just unnecessarily copied from the old version?
use size-* classes to style
Sorry @lc-thomasberger but it looks like something's gone a bit wrong with the _hasNavigationInTextArea layout following that last set of updates: |
@moloko it is this line in the theme https://github.com/adaptlearning/adapt-contrib-vanilla/blob/master/less/src/narrative.less#L44 |
@lc-thomasberger OK so from what I can see it's this which is overriding this in the theme. For reference, this is how it how it looks in the current version of narrative when (As I recall, it was decided that we'd have the So I think all that needs to be done is to remove these lines and we're then back to the same layout we have currently. |
change to narrative.less has caused a regression in the _hasNavigationInTextArea layout
use ItemsComponentModel
improve animation performance