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

Add setting to auto load comment section #3352

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Mar 24, 2023

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

N/A

Description

Add a setting to allow comment section to be auto loaded, default disabled
When enabled (with a percentage), comment section bottom when appears pass N% above bottom of viewport will have comment data auto loaded once (won't load it if data already being loaded, loaded)

Screenshots

New setting position (can be adjusted)
image

Testing

A. Disabled

  • No setting change should be required, it's default
  • View a video with comment
  • Scroll~~~
  • Ensure comment not auto loaded

B. 0% from window/view port bottom

  • Update setting
  • View a video with multiple pages of comment (e.g. https://youtu.be/h-qHtElnTbE)
  • Scroll carefully and observe comment section
  • Ensure comments auto loaded once comment section bottom is 0% above bottom of window/view port
  • Scroll down again
  • Ensure more comments auto loaded

You can test other settings as well, those percentage values (yes zero or negative values) are just passed into IntersectionObserver rootMargin

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

  • To prevent comment autoload on page load (when video player is minimized for a moment), a timeout is used (1000ms)
    I am not sure if that's good enough
  • Updating setting won't adjust behaviour until video switched

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Mar 24, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) March 24, 2023 03:31
@ChunkyProgrammer
Copy link
Member

I think a toggle should probably be used instead (percentage might be confusing for users)

@PikachuEXE
Copy link
Collaborator Author

PikachuEXE commented Mar 24, 2023

I don't understand
Toggle to enable and another setting to adjust percentage?

Note: Percentage is required to adjust to user device screen size & preference (on window size)

@absidue
Copy link
Member

absidue commented Mar 24, 2023

Chunky means don't expose any percentages to the user, pick one and then have a single on and off switch.

Also instead of writing your own IntersectionObserver code, you can use vue-observe-visibility, which we already have to lazy load the videos etc in the lists. See ft-list-lazy-wrapper to see how it's used there.

@PikachuEXE
Copy link
Collaborator Author

Problem is

  • small number like 0% = Auto load on large windows anyway
  • large number = inconvenient

Let me take a look at vue-observe-visibility...

@absidue
Copy link
Member

absidue commented Mar 24, 2023

If the comment section is always visible in large windows people will expect it to auto load straight away anyway.

@PikachuEXE
Copy link
Collaborator Author

Updated

static/locales/en-US.yaml Outdated Show resolved Hide resolved
src/renderer/components/player-settings/player-settings.js Outdated Show resolved Hide resolved
src/renderer/components/player-settings/player-settings.js Outdated Show resolved Hide resolved
src/renderer/store/modules/settings.js Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to autoload comments when i almost reach the 'load more comments' button

@PikachuEXE
Copy link
Collaborator Author

It would be nice to autoload comments when i almost reach the 'load more comments' button
Do you mean mouse cursor?

@PikachuEXE
Copy link
Collaborator Author

Unnecessary translation entries removed

Copy link
Collaborator Author

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

It's my habit to ensure the values in DB is more flexible than the UI when possibility of future change is present

return this.$store.getters.getCommentAutoLoadCondition
},

commentAutoLoadConditionEnabled: function () {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using string is intentional even though UI is a toggle
I am not excluding the possibility to introduce other values later
No DB change needed when that's the case

@@ -266,6 +274,12 @@ export default defineComponent({
})
},

updateCommentAutoLoadConditionEnabled: function(enabled) {
this.updateCommentAutoLoadCondition(
enabled ? '0%' : 'disabled'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Intentional "frontend boolean to backend enum" conversion

@efb4f5ff-1298-471a-8973-3d47447115dc

It would be nice to autoload comments when i almost reach the 'load more comments' button
Do you mean mouse cursor?

When i reach the bottom of the comment section i want it to auto load more comments

VirtualBoxVM_R1XNzfC1mb.mp4

@PikachuEXE
Copy link
Collaborator Author

PikachuEXE commented Mar 26, 2023

Different feature, different PR please
Update 1: Or do you mean it should be done in this PR too???
Update 2: Will try to implement it if not merged within 2 days

@absidue
Copy link
Member

absidue commented Mar 26, 2023

@PikachuEXE I doubt we'll be adding more values in the future, not having it be a boolean sounds like unnecessary complication to me.

Although considering that in most screenshots you've sent, you have had FreeTube at a massive window size, I'm guessing that the reason you don't want to switch to a boolean, is so that you can have a different percentage in your personal settings db, while everyone else uses 0%.

@efb4f5ff-1298-471a-8973-3d47447115dc

Different feature, different PR please Update 1: Or do you mean it should be done in this PR too??? Update 2: Will try to implement it if not merged within 2 days

As user i expect the following when i enable this feature:

  1. When i navigate to a video it loads the comments section (current behavior of PR)
  2. When i scroll down the comment section it should load more comments

IMO (again from an user perspective) the PR in its current iteration is a half baked feature. So to come back to your question i dont see this as a different feature.

@absidue absidue added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Mar 26, 2023
@PikachuEXE
Copy link
Collaborator Author

Is there a path to update the setting from boolean to string if it ever happens?
Update reader code?

Also are 4 new methods instead of 2 really that complicated?

@absidue
Copy link
Member

absidue commented Mar 26, 2023

I just don't think it's necessary to over engineer something, just in case we change our minds in the future, as that is extremely unlikely to happen.

@PikachuEXE
Copy link
Collaborator Author

Updated

@PikachuEXE PikachuEXE added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Mar 26, 2023
@PikachuEXE PikachuEXE force-pushed the feature/video-comment-auto-load-on-scroll branch from beaa8cf to 31d16a0 Compare April 8, 2023 06:48
@PikachuEXE
Copy link
Collaborator Author

Setting updated

Also removed the delay on comment loading (I didn't know there is a ready event)

@PikachuEXE PikachuEXE requested a review from absidue April 8, 2023 10:59
@efb4f5ff-1298-471a-8973-3d47447115dc

it shows being greyed out but enabled. That could potentially be confusing so i think u should also disable it.. AFAIK we also do that with other settings like that

@PikachuEXE
Copy link
Collaborator Author

Done

@PikachuEXE PikachuEXE requested review from absidue and ChunkyProgrammer and removed request for absidue and ChunkyProgrammer April 9, 2023 23:38
@efb4f5ff-1298-471a-8973-3d47447115dc

@ChunkyProgrammer ur powers are needed

@FreeTubeBot FreeTubeBot merged commit b5e35d0 into FreeTubeApp:development Apr 27, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 27, 2023
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.

5 participants