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

Only update track mode if changed #4298

Merged
merged 4 commits into from
May 12, 2017
Merged

Only update track mode if changed #4298

merged 4 commits into from
May 12, 2017

Conversation

arski
Copy link
Contributor

@arski arski commented Apr 20, 2017

Hi there,

Would you consider this fix for triggering less track.modechange and subsequently trackList.change events - at the moment too many of those are triggered even if nothing has actually changed.

cc #4294

@arski
Copy link
Contributor Author

arski commented Apr 20, 2017

cc @gkatsev what do you think

track.mode = 'disabled';
if (track.mode !== 'disabled') {
track.mode = 'disabled';
}
Copy link
Member

Choose a reason for hiding this comment

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

The linter is complaining about this not being combined with the else, like so:

} else if (track.mode !== 'disabled') {

@gkatsev gkatsev added this to Ready to Review in video.js May 8, 2017
@gkatsev gkatsev moved this from Ready to Review to Reviewed, awaiting updates in video.js May 8, 2017
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

This is good to go assuming tests and linter passed. I updated the PR by consolidated the if statements.

@gkatsev gkatsev merged commit 3087830 into videojs:master May 12, 2017
@gkatsev
Copy link
Member

gkatsev commented May 12, 2017

@arski thanks! Would you be able to make a similar PR against the 5.x branch? I can take care of that otherwise.

@gkatsev gkatsev moved this from Reviewed, awaiting updates to Done in video.js May 12, 2017
@arski
Copy link
Contributor Author

arski commented May 18, 2017

@gkatsev thanks! would you mind doing against the 5.x branch? Im not sure when ill get to it :(

btw how frequently do you bump versions for the lib?

@gkatsev
Copy link
Member

gkatsev commented May 18, 2017

Sure, I'll make a PR for it.

As for how often, it really just depends on whether there's things to release, we don't really have a strict release schedule right now, though, I want to try and aim for regular releases. 6.x with this fix has been released already.

@arski
Copy link
Contributor Author

arski commented May 18, 2017

Aha nice, good to know!

@gkatsev
Copy link
Member

gkatsev commented May 18, 2017

Made a PR against 5.x in #4368.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants