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 Duplicate loading animation #468

Merged
merged 5 commits into from Sep 30, 2019

Conversation

grafixeyehero
Copy link
Contributor

Issues
Duplicate loading animation #199

Copy link
Contributor

@JustAMan JustAMan left a comment

Choose a reason for hiding this comment

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

Looks mostly fine, but I am slightly wary of breaking things. Did you test major browsers?

src/components/htmlvideoplayer/plugin.js Outdated Show resolved Hide resolved
@grafixeyehero
Copy link
Contributor Author

Wrong btn sorry

@grafixeyehero
Copy link
Contributor Author

Tested
Chrome 76.0.3809.132
Firefox 70.0b6

Copy link
Contributor

@JustAMan JustAMan left a comment

Choose a reason for hiding this comment

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

So you basically flipped the condition? Maybe do just that instead of exchanging two lines, so that the diff is the smallest? (looks like same effect could be achieved just by deleting ! in the if())

Copy link
Contributor

@JustAMan JustAMan left a comment

Choose a reason for hiding this comment

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

I hope you have tested this. Awesome fix!

@anthonylavado anthonylavado requested a review from a team September 19, 2019 18:37
@anthonylavado anthonylavado added this to To Do in Release 10.4.0 via automation Sep 19, 2019
@dkanada
Copy link
Member

dkanada commented Sep 20, 2019

This adds a duplicate loading animation for firefox on my machine. I definitely want to merge this once we figure out the behavior but this doesn't seem like the right solution.

@grafixeyehero
Copy link
Contributor Author

grafixeyehero commented Sep 21, 2019

This adds a duplicate loading animation for firefox on my machine. I definitely want to merge this once we figure out the behavior but this doesn't seem like the right solution.

the first comment didn't hide on Firefox. i hope this fix switching condition 20c23dc

Release 10.4.0 automation moved this from To Do to To Merge Sep 24, 2019
anthonylavado
anthonylavado previously approved these changes Sep 24, 2019
Copy link
Member

@dkanada dkanada left a comment

Choose a reason for hiding this comment

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

Still broken on two out of three browsers I used for testing. One has a Chromium base and the other is Firefox, so they aren't uncommon.

@anthonylavado anthonylavado moved this from To Merge to To Do in Release 10.4.0 Sep 26, 2019
@grafixeyehero
Copy link
Contributor Author

@dkanada could you explain me a in which version you test and the problem that you faced. is it the controller or the loading animation

Copy link
Contributor

@JustAMan JustAMan left a comment

Choose a reason for hiding this comment

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

Wow, this looks clever! Hope this works.

@joshuaboniface joshuaboniface removed this from To Do in Release 10.4.0 Sep 29, 2019
Copy link
Member

@dkanada dkanada left a comment

Choose a reason for hiding this comment

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

I'm sure there was a reason they added the check, but it seems to work fine without the controls so we're probably fine. I tested this on Firefox, Chromium, and a very uncommon browser and it worked on all three.

@dkanada dkanada merged commit e7610b0 into jellyfin:master Sep 30, 2019
@grafixeyehero grafixeyehero deleted the htmlvideoplayer branch October 2, 2019 14:34
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

5 participants