search

brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.79k stars 2.32k forks source link

Playlist should confirm deletion of playlists, per Figma #40248

Open stephendonner opened 2 months ago

stephendonner commented 2 months ago

Description

Playlist should confirm deletion of playlists, per Figma

Steps to reproduce

  1. install 1.70.41
  2. launch Brave
  3. restart Brave (to pick up Griffin seeds for Playlist)
  4. add a media file/video to your playlist
  5. select the media file
  6. hover over and click on the ... 3-dots menu
  7. choose Remove from playlist

Actual result

Nothing visually happens (other than the media file is instantly deleted)

Expected result

example example
Screen Shot 2024-08-02 at 5 57 17 PM Screen Shot 2024-08-02 at 5 57 25 PM

Reproduces how often

Easily reproduced

Brave version (brave://version info)

<img width="381" alt="Screen Shot 2024-08-02 at 5 57 25 PM" src="https://github.com/user-attachments/assets/d07948fd-12c0-4ffd-8748-ab1d7f8c3b4b">
Brave | 1.70.41 Chromium: 127.0.6533.88 (Official Build) nightly (x86_64)
-- | --
Revision | 9303bae9fe07bc2ec7c0cdef139e87304b2d6c96
OS | macOS Version 11.7.10 (Build 20G1427)

Channel information

Reproducibility

Miscellaneous information

cc @aguscruiz @rebron @sangwoo108 @szilardszaloki @brave/qa-team

sangwoo108 commented 2 months ago

Hi, could you try removing Playlist folder, not individual media items? I thought the warning message is for that. cc @aguscruiz

aguscruiz commented 2 months ago

I wouldn't be against adding a dialog for deleting individual items too, although may be a bit annoying after a while. What do you think @rebron ?

stephendonner commented 2 months ago

Not sure if this prompt should look like Figma?

Screen Shot 2024-08-05 at 10 41 33 AM
aguscruiz commented 2 months ago

Yeah, there should be a maximum width established. I think I used the same width as other dialogs that appear in the browser. But this one is too wide.

I have a 512px max limit set up on the file

rebron commented 2 months ago

I wouldn't be against adding a dialog for deleting individual items too, although may be a bit annoying after a while. What do you think @rebron ?

If we can hook up Undo, I don't think we need a delete confirmation dialog for individual items.