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 confirmation popup before unsubscribing #4896

Merged
merged 6 commits into from
Jun 11, 2024

Conversation

msagr
Copy link
Contributor

@msagr msagr commented Apr 7, 2024

Added a popup to ask for confirmation before unsubscribing

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #4109

Description

It asks for a confirmation while unsubscribing channel, to avoid accidental unsubscription.

Screenshots

Screenshot from 2024-04-08 01-18-53

Testing

I tested it by first subscribing various channels, and everytime I unsubscribed a channel, it would ask me for a confirmation that if I really want to unsubscribe.

Desktop

  • **OS:**Ubuntu
  • **OS Version:**22.04
  • **FreeTube version:**0.20.0

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 7, 2024 19:53
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 7, 2024
auto-merge was automatically disabled April 7, 2024 23:09

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 7, 2024 23:10
auto-merge was automatically disabled April 7, 2024 23:20

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 7, 2024 23:20
auto-merge was automatically disabled April 7, 2024 23:32

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 7, 2024 23:33
auto-merge was automatically disabled April 7, 2024 23:33

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 7, 2024 23:34
@efb4f5ff-1298-471a-8973-3d47447115dc

I like the way you implemented it but im not sure if the issue has valid reasoning for this to exist. Now if you need to unsubscribe one channel from lets say 3 profiles you are encountering this 3 times. Do the same routine for 10 more channels and this will get very annoying very quickly.

If some of the other reviewers think this is a valid PR than there should be a toggle for this confirmation in the settings (maybe parental control idk)

@absidue
Copy link
Member

absidue commented Apr 9, 2024

If we do decide to keep this, then it should be changed to use the ft-prompt component so that it matches the popups throughout the rest of the app.

@PikachuEXE
Copy link
Collaborator

I think there should be a toggle for this

@msagr
Copy link
Contributor Author

msagr commented Apr 9, 2024

Yeah, I think that it makes more sense to have a separate option in setting like avoid accidental unsubscription which when enabled, then only the user will be shown popup while unsubscribing. Should I try to implement it this way?

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

@msagr yes please implement the suggested changes :)

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Apr 12, 2024
auto-merge was automatically disabled April 13, 2024 08:21

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 13, 2024 08:21
@msagr
Copy link
Contributor Author

msagr commented Apr 13, 2024

Screencast.from.13-04-24.01.48.16.PM.IST.webm

Copy link
Member

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

Choose a reason for hiding this comment

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

Dropdown should not close after i removed the channel from a profile

VirtualBoxVM_7ZveIUYxNN.mp4

popup playing hide and seek on the player page

VirtualBoxVM_v5CRDh3dEI.mp4

@msagr
Copy link
Contributor Author

msagr commented Apr 15, 2024

Thanks @efb4f5ff-1298-471a-8973-3d47447115dc for the review, I will try to implement the suggested changes.

@msagr
Copy link
Contributor Author

msagr commented Apr 17, 2024

@efb4f5ff-1298-471a-8973-3d47447115dc, I was trying to move the side videopanel out of focus when the prompt pops up but i am facing difficulty in doing that. Can you please suggest me something?

@kommunarr
Copy link
Collaborator

kommunarr commented Apr 20, 2024

popup playing hide and seek on the player page

I've tested merging this with #3975 and am no longer seeing this bug here. Do we want to wait on this PR until that one is merged?

Screenshot_20240420_082313

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Apr 22, 2024

Key difference from you screenshot vs mine

Your player is in theatre mode and mine isnt. Did you test this with both modes?

@kommunarr
Copy link
Collaborator

kommunarr commented Apr 22, 2024

Edit: sorry for the temporary laziness. Yes, it works with both modes:

Screenshot_20240422_170208

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

Approving after #3975 is merged

@kommunarr kommunarr added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR/Issue: dependent labels May 1, 2024
@kommunarr
Copy link
Collaborator

Needs to be rebased with development, and then it should be working as intended

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

@jasonhenriquez should #4374 be merged first?

@kommunarr
Copy link
Collaborator

It's arguable, so maybe yes. It's the least destructive action we could have, and easily reversible at that, but should still probably be marked properly given the use case it's trying to solve for (accidental unsubscribing)

Copy link
Member

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

Choose a reason for hiding this comment

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

Both PR's are merged. Rebase into development branch

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 2, 2024
Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@zipfile6209
Copy link

zipfile6209 commented Jun 8, 2024

Unfortunately it looks like @msagr is busy, it would be a pity if his cool work was lost.

I don't know about programming but I gather that all the work is done and that bringing it to the current version could be very easy. Would it be impolite to ask if another dev could put the finishing touches on it?

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

The devs are people with very limited time because we all do this in our spare time. If we had to take over every PR that gets abandoned we would never get to the important stuff.

We have way more important stuff todo with our limited time than addressing low priority issues like this one.

New contributors feel free to take the code of the PR and make the necessary changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested PR: stale labels Jun 11, 2024
Copy link
Collaborator

@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.

Tested after merging locally (before OP merged dev branch recently)

@FreeTubeBot FreeTubeBot merged commit 27d2687 into FreeTubeApp:development Jun 11, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 11, 2024
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.

[Feature Request]: Confirmation to remove subscriptions
8 participants