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.

Sign up for GitHub

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

Jump to bottom

Frontend: Add video timestamp on external links #4101

Merged
merged 7 commits into from
Oct 21, 2023
Merged

Frontend: Add video timestamp on external links #4101

merged 7 commits into from
Oct 21, 2023

Conversation

SamantazFox
Copy link
Member

@SamantazFox SamantazFox commented Sep 14, 2023 •
edited
Loading

Closes #3089
Improves the situation for #3348

@SamantazFox SamantazFox requested a review from a team as a code owner September 14, 2023 20:28
@SamantazFox SamantazFox requested review from unixfox and removed request for a team September 14, 2023 20:28
syeopite
syeopite requested changes Sep 14, 2023
View reviewed changes
assets/js/player.js Outdated Show resolved Hide resolved
assets/js/player.js Outdated Show resolved Hide resolved
syeopite
syeopite reviewed Sep 16, 2023
View reviewed changes
assets/js/player.js Outdated Show resolved Hide resolved
syeopite
syeopite requested changes Sep 16, 2023
View reviewed changes
assets/js/player.js Outdated Show resolved Hide resolved
assets/js/player.js Outdated Show resolved Hide resolved
@SamantazFox @syeopite
JS: Replace '.setAttribute("href")' with '.href'
700c575 
Co-authored-by: syeopite <70992037+syeopite@users.noreply.github.com>
absidue
absidue approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@absidue absidue left a comment

Choose a reason for hiding this comment

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

The JavaScript code looks good, I don't know crystal so I haven't reviewed that and I also don't have an Invidious development environment set up, so I haven't tested it either.

@SamantazFox
Copy link
Member Author

@syeopite In the end, I've done it properly. It now updates exactly once per second.
The previous code was bogus anyway, as it would update every 15ms for 1s, then don't update for 4s, and so on.

@SamantazFox SamantazFox added the in-testing This feature has been deployed and is being tested label Oct 21, 2023
@SamantazFox
Copy link
Member Author

@absidue it's deployed on the test instance if you want.

absidue
absidue approved these changes Oct 21, 2023
View reviewed changes
Copy link
Contributor

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Seems good to me on the test instance.

@SamantazFox SamantazFox added ready and removed in-testing This feature has been deployed and is being tested labels Oct 21, 2023
@SamantazFox SamantazFox merged commit 2a65b5f into iv-org:master Oct 21, 2023
7 checks passed
@SamantazFox SamantazFox deleted the timestamp-ext-links branch October 21, 2023 16:36
@syeopite
Copy link
Member

The timestamp parameter is ignored when automatic instance redirection is enabled. I guess a quick and dirty fix would be to just automatically include any additional URL parameters in the /redirect endpoint.

@alondrayona alondrayona mentioned this pull request Oct 23, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@absidue absidue absidue approved these changes

@unixfox unixfox Awaiting requested review from unixfox unixfox is a code owner automatically assigned from iv-org/developers