Skip to content

duckplayer fixes #619

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
Merged
merged 1 commit into from
Jul 11, 2023
Merged

duckplayer fixes #619

merged 1 commit into from
Jul 11, 2023

Conversation

shakyShane
Copy link
Contributor

@shakyShane shakyShane commented Jul 10, 2023 •
edited
Loading

https://app.asana.com/0/0/1205002113935587/f

This fixes 2 things:

  • 1) A previous fix introduced a regression where we always called e.preventDefault() when we shouldn't
  • 2) YT upgrades the markup on the elements for shorts, so now we double-check on click whether or not it's a valid duck player url before we prevent default and try to navigate.

@shakyShane
Copy link
Contributor Author

shakyShane commented Jul 10, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@shakyShane shakyShane mentioned this pull request Jul 11, 2023
Merged
@shakyShane shakyShane force-pushed the shane/duckplayer-shorts-enabled branch from 40a25d1 to b34afb6 Compare July 11, 2023 14:40
@shakyShane shakyShane force-pushed the shane/duckplayer-shorts-enabled branch from 58a4005 to f4a8eb2 Compare July 11, 2023 14:56
@shakyShane shakyShane changed the title wip: duckplayer fixes duckplayer fixes Jul 11, 2023
jonathanKingston
jonathanKingston reviewed Jul 11, 2023
View reviewed changes
@shakyShane shakyShane force-pushed the shane/duckplayer-shorts-enabled branch from 23c0101 to f4a8eb2 Compare July 11, 2023 17:30
@shakyShane shakyShane force-pushed the shane/duckplayer-shorts-enabled branch from f4a8eb2 to 234dc84 Compare July 11, 2023 17:31
jonathanKingston
jonathanKingston approved these changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@jonathanKingston jonathanKingston left a comment

Choose a reason for hiding this comment

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

As discussed this is more robust than the existing code as we're not caching the href.

We're removing the hover code as this was a YT experiment.

@shakyShane shakyShane merged commit c993278 into main Jul 11, 2023
@shakyShane shakyShane deleted the shane/duckplayer-shorts-enabled branch July 11, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathanKingston jonathanKingston jonathanKingston approved these changes

@q71114 q71114 Awaiting requested review from q71114

@szanto90balazs szanto90balazs Awaiting requested review from szanto90balazs

Assignees

@jonathanKingston jonathanKingston

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shakyShane @jonathanKingston