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

Created Undo Button On Toast After Removing Video From Playlist. #5845

Closed
wants to merge 0 commits into from

Conversation

Soham456
Copy link

@Soham456 Soham456 commented Oct 9, 2024

Pull Request Type

  • [ X ] Feature Implementation

Related issue

"closes #5421"

Description

This pr is a new feature which gives an 'undo' option for user to add removed video back to playlist within 3 seconds of removal , just incase it is accidently removed.

Additional context

This is my first ever pull request please let me know if anything is wrong or can be improved.

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 9, 2024 14:40
@github-actions github-actions bot added PR: dependencies Pull requests that update a dependency file PR: waiting for review For PRs that are complete, tested, and ready for review labels Oct 9, 2024
@ChunkyProgrammer
Copy link
Member

Please undo your changes to yarn.lock and remove package-lock.json

@@ -107,7 +107,7 @@
:initial-visible-state="index < 10"
@move-video-up="moveVideoUp(item.videoId, item.playlistItemId)"
@move-video-down="moveVideoDown(item.videoId, item.playlistItemId)"
@remove-from-playlist="removeVideoFromPlaylist(item.videoId, item.playlistItemId)"
@remove-from-playlist="removeVideoFromPlaylist(item.videoId)"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can pass item here so you dont need to find the video in the remove function?

Copy link
Author

Choose a reason for hiding this comment

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

item.playlistItemId is giving null value

showToast(this.$t('User Playlists.SinglePlaylistView.Toast.Video has been removed'), 3000, () => {
this.addVideoBackToPlaylist(videoData);
},
'Undo'
Copy link
Member

Choose a reason for hiding this comment

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

This would need to be translated. I think we might already have a translation key for this so it'd just be

Suggested change
'Undo'
this.$t('Undo')

});
// Update playlist's `lastUpdatedAt`
this.updatePlaylist({ _id: this.playlistId });
showToast('Video has been added back to playlist', 3000);
Copy link
Member

Choose a reason for hiding this comment

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

This also needs to be translated

this.updatePlaylist({ _id: this.playlistId });
showToast('Video has been added back to playlist', 3000);
} catch (e) {
showToast('There was a problem with adding this video back to playlist');
Copy link
Member

Choose a reason for hiding this comment

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

This also needs to be translated

@@ -7,13 +7,18 @@
:class="{ closed: !toast.isOpen, open: toast.isOpen }"
tabindex="0"
role="status"
@click="performAction(toast.id)"
Copy link
Member

Choose a reason for hiding this comment

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

I believe removing this will break the ability to copy error messages for error toasts

Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

This pull request is way too over-engineered and as ChunkyProgrammer mentioned, this breaks lots of existing functionality (copying errors, undoing the change of the quick bookmark playlist, etc etc).

Please implement the undo in the same way that it is done for the quick bookmark feature (just changing the text and adding a action function that undoes the change): https://github.com/FreeTubeApp/FreeTube/blob/development/src/renderer/components/playlist-info/playlist-info.js#L515-L529

@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 Oct 9, 2024
auto-merge was automatically disabled October 11, 2024 11:28

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 11, 2024 11:28
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Soham456
Copy link
Author

hey , can you help ? how should i resolve this conflict in yarn.lock file?

@absidue
Copy link
Member

absidue commented Oct 15, 2024

Remove all changes that you made to it, as this pull request doesn't change any dependencies it should not include any modifications to the yarn.lock file.

auto-merge was automatically disabled October 17, 2024 09:06

Head branch was pushed to by a user without write access

@Soham456 Soham456 closed this Oct 17, 2024
auto-merge was automatically disabled October 17, 2024 09:06

Pull request was closed

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.

[Feature Request]: Undo for "Remove from Playlist"
4 participants