Skip to content

[ext] feat: display a feedback to the user when the end of history is reached#2063

Closed
GresilleSiffle wants to merge 2 commits intorate-later-historyfrom
rate-later-history_finish_feedback
Closed

[ext] feat: display a feedback to the user when the end of history is reached#2063
GresilleSiffle wants to merge 2 commits intorate-later-historyfrom
rate-later-history_finish_feedback

Conversation

@GresilleSiffle
Copy link
Collaborator

@GresilleSiffle GresilleSiffle commented Feb 27, 2025

target PR #2059


Description

Display a visual feedback to the users when the import is complete.

capture

Checklist

  • I added the related issue(s) id in the related issues section (if any)
    • if not, delete the related issues section
  • I described my changes and my decisions in the PR description
  • I read the development guidelines of the CONTRIBUTING.md
  • The tests pass and have been updated if relevant
  • The code quality check pass

@GresilleSiffle GresilleSiffle added the Extension Development of the browser extension label Feb 27, 2025
@GresilleSiffle GresilleSiffle self-assigned this Feb 27, 2025
@GresilleSiffle GresilleSiffle changed the title feat: display a feedback to the user when the end of history is reached [ext] feat: display a feedback to the user when the end of history is reached Feb 27, 2025
Copy link
Collaborator

@sigmike sigmike left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. I'm a little concerned about the way we detect the end of the loading process. If there's a network lag or anything slowing down the loading for some time, we will assume the loading is complete. It won't stop the process though, so the user will still see more videos loading.

My YouTube watch history is too big to reach the end in a reasonable time frame, so I can't really test it.

If it works for you and you don't mind having some false completions detected, I'm fine with merging this.

let timeout;

const captureStep = async () => {
const captureStep = async (onEndOfHistory) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you choose to pass this as argument? This function already uses several variables defined in its parent scope.

@GresilleSiffle
Copy link
Collaborator Author

I'm a little concerned about the way we detect the end of the loading process. If there's a network lag or anything slowing down the loading for some time, we will assume the loading is complete.

You're right. The check is not robust enough, I didn't think about possible network slowdowns sorry.

If it works for you and you don't mind having some false completions detected, I'm fine with merging this

I'd prefer to avoid introducing erroneous user feedback on this feature. We can totally close this PR.

My initial goal was to add visual cues, to invite the users to click on Stop when the numbers of visible and imported videos are equal. Maybe a small improvement could be to display those number in bold, to make them sand out from the rest of the text. What do you think?

@sigmike
Copy link
Collaborator

sigmike commented Mar 3, 2025

My initial goal was to add visual cues, to invite the users to click on Stop when the numbers of visible and imported videos are equal. Maybe a small improvement could be to display those number in bold, to make them sand out from the rest of the text. What do you think?

Sounds good. I made the change in the main PR.

If you want to let the user know that nothing has changed for a while, we could display the elapsed time since the last change of the video count. But it may require a small explanation text, so I'm not sure it's actually easier for the user.

@GresilleSiffle
Copy link
Collaborator Author

But it may require a small explanation text, so I'm not sure it's actually easier for the user.

You're right. An extra explanation would make the UI more "heavy" I think. Let's keep it as is for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Extension Development of the browser extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants