Skip to content

Use native web browser features instead of jquery #2360

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

Closed
wants to merge 1 commit into from

Conversation

absidue
Copy link
Member

@absidue absidue commented Jun 25, 2022


Use native webbrowser features instead of jquery

Pull Request Type

  • Feature Implementation

Related issue
Please link the issue your pull request is referring to. If this pull request fully resolves the relevant issue, put "closes" before the issue number. Example: "closes #123456".

Description
Due to jquery's popularity most of jquery's features have now become web standards and are implemented into web browsers natively. This pull request replaces all uses of jquery in FreeTube with their web standards counter parts.

Here are some of the changes that I made:

replace $.getJson() with fetch() (docs)

$.getJSON(requestUrl, (response) => {
  // ...
}).fail((xhr, textStatus, error) => {
  // ...
})

fetch(requestUrl)
  .then((response) => response.json())
  .then((json) => {
    // ...
  })
  .catch((error) => {
    // ...
  })

replace $() and $().get(0) with document.getElementById() (docs), document.querySelector() (docs) and document.querySelectorAll() (docs)

$('#theId') /* -> */ document.getElementById('theId')

// selecting the first element matching a css query
$('.class').get(0) /* -> */ document.querySelector('.class')

// selecting all elements that match a css query
$('.class') /* -> */ document.querySelectorAll('.class')

adding event listeners:
replace .on() with .addEventListener() (docs)

$('#profileList').on('focusout', (event) =>{
  // ...
})

document.getElementById('profileList').addEventListener('focusout', (event) => {
  // ...
})

replace .addClass() and .removeClass() with .classList.add() (docs) and .classList.remove() (docs)

$('#loopButton').removeClass('vjs-icon-loop-active')
$('#loopButton').removeClass('loop-black')
$('#loopButton').addClass('loop-white')

const loopButton = document.getElementById('loopButton')
loopButton.classList.remove('vjs-icon-loop-active')
loopButton.classList.remove('loop-black')
loopButton.classList.add('loop-white')

replace .animate({ scrollTop: /* ... */}) with .scrollTo({ top: /* ... */ }) (docs)

liveChatComments.animate({ scrollTop: liveChatComments.prop('scrollHeight') })
liveChatComments.scrollTo({ top: liveChatComments.scrollHeight })
@media (prefers-reduced-motion: no-preference) {
  .liveChatComments {
    scroll-behavior: smooth;
  }
}

Testing
Everything should work the same as it did before this PR. It's a good idea to have an instance of the nightly build open, so you can compare the before and after.

  • checking for updates
    1. change the version to 0.15.0 in the package.json file
    2. check that the update available banner shows up
    3. check that the changelog shows up
  • keyboard shortcuts (e.g. ALT+D to focus the search bar)
  • clicking links in video descriptions, comments etc
  • profile selector
    1. check that it opens correctly
    2. check that it closes when it looses focus
    3. check that it closes when a profile is selected
  • video player
    • keyboard shortcuts
    • the highlighted quality is corrent and changes correctly in the quality selector
    • check that the loop, full window, theatre and screenshot buttons look and function as expected
    • captions
    • sponsor block segments
  • Invidious API as backend
  • history navigation arrows get updated correctly
  • the search bar gets hidden/shown on mobile/in narrow windows
  • proxy test button in settings
  • live chat
    1. set the backend to the local API (invidious API doesn't support live chat)
    2. find a live stream (some are listed here https://www.youtube.com/live)
    3. the live chat will auto scroll as new comments come it

Desktop (please complete the following information):

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 6344228

Additional context
The live chat stops smooth scrolling after a while, this seems to be a chromium/electron limitation, presumably some kind of power saving measure. Thankfully it keeps scrolling, it just jumps to the latest message instead of animating the scroll, manually scrolling still works smoothly, so this only affects scrolling programatically.

@absidue absidue added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 25, 2022
@PrestonN PrestonN enabled auto-merge (squash) June 25, 2022 13:37
@ChunkyProgrammer
Copy link
Member

ChunkyProgrammer commented Jun 25, 2022

I think this is a good change. I think updating node to 18 might need to happen, however as that's when fetch is fully supported in node (i'm not sure if browser fetch is used for all requests or if node fetch is being used for some).

If node does need to be updated then I think updating the workflows to use node 18 & updating babel for node 18 would be a good idea. Whether included in this PR or a separate one. Note: I have not yet tested this PR but I looked at the code changes and the code looks good to me so far.
image
source: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API

I would also like to note that 16 is the version that node is currently recommending for most users so updating a development document might be a good idea if node 18 is required:
https://github.com/FreeTubeApp/FreeTube-Docs/blob/master/development/getting-started.md

image

@absidue
Copy link
Member Author

absidue commented Jun 25, 2022

@ChunkyProgrammer As far as I can tell all of these fetch calls are run in the chromium part of electron not the node part, as they all work without updating node or electron. Electron 19 still uses node 16 according to the release notes, so 17 and 18 probably do too.

This PR shouldn't need any updates to electron or node to work.

@PikachuEXE
Copy link
Collaborator

I would prioritize this below electron upgrade (to 17/18)

I would still support this change but I think we don't have time to test so many changes at once

@absidue absidue added the PR: dependencies Pull requests that update a dependency file label Jul 1, 2022
@ChunkyProgrammer
Copy link
Member

I would also like to see #1525 merged before this PR

@absidue
Copy link
Member Author

absidue commented Aug 7, 2022

I'll wait until the accessibility PR is merged before fixing the merge conflicts.

@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 6, 2022
auto-merge was automatically disabled October 6, 2022 16:33

Pull request was closed

@absidue absidue deleted the remove-query branch October 7, 2022 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants