Closed Bug 1168130 Opened 5 years ago Closed 5 years ago
Allow deleting of individual Top Sites Tiles
552.71 KB, image/png
6.57 KB, application/zip
47 bytes, text/x-github-pull-request
|Details | Review|
86 bytes, patch
|Details | Diff | Splinter Review|
Tapping and holding should surface an 'x' by each tile, which by tapping deletes the entry from the list. Will also need a 'done' mechanism to return to prior state. Mockups to come.
Could you hook me up with the various Close button assets? Probably be best to keep the x + red circle as it's own image.
Now with 19% more opacity!
Attachment #8616109 - Attachment is obsolete: true
First pass at implementing deletion of top site tiles form the top sites panel. Matches android implementation where when tiles are deleted, the history associated with that tile is also deleted.
Comment on attachment 8616757 [details] [review] PR https://github.com/mozilla/firefox-ios/pull/572 Some minor comments in the PR. Works great!
Attachment #8616757 - Flags: review?(bnicholson) → review+
I stumbled on some issues with the way I was inserting/deleting items from the collection view which was causing crashes and also bugs with the way the decorator view was rendering when updating the layout. I moved the remove button into the ThumbnailCell so it's easier to animate and keep on top of the thumbnail image and fixed up the logic for inserting and deleting cells. Mind taking a look at the revised changes? Also, I've disabled deleting suggested tiles for now as they will always come back since we're seeding the collection view with suggested tiles if there isn't anything available. There seems to be a lot more thinking that needs to go into 'removing' the tiles but I think this is a good start for now.
Attachment #8617465 - Flags: review?(bnicholson)
Comment on attachment 8617465 [details] [diff] [review] Commit https://github.com/mozilla/firefox-ios/commit/b0c1435eedf14ae47fbada871432e410a5a8a6ff This is working pretty well for me. One case I noticed that's broken: scrolling the list down to hide some of the thumbnails, then long-pressing, then scrolling back up. After these steps, the close button doesn't appear on the thumbnails that come back into view (this worked correctly on the first version of your PR). One other question: I noticed long-pressing the list of sites below the thumbnails also triggers the close buttons, but the buttons only appear for the thumbnails above the list. Should we change it so only the thumbnails area is long-pressable? If I'm farther down in the list without any thumbnails visible, then I long press, the only thing that happens is the Done toolbar appearing at the top, which is kind of confusing. Other than that, looks fine to me.
Attachment #8617465 - Flags: review?(bnicholson) → review+
One other (less important) detail: we might want to turn off editing if the user taps the URL bar or opens the tab tray; it feels strange coming back to the Top Sites panel with editing still active. I'd be OK if this was fixed as a separate follow-up, though.
Thanks for taking a look at the changes. I've fixed the issues you mentioned above and I'll follow up with a patch to end editing when tapping the URL bar. I'll go ahead and merge this in then.
Created https://bugzilla.mozilla.org/show_bug.cgi?id=1173372 to track URL bar tapping issue. Merged.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8616757 - Flags: feedback?(dhenein) → feedback+
I'm wondering what future interactions can be applied now with the inclusion of this addition. Long-tap on a top-site and open in background tab or bookmark (or anything in a context menu) can't be used.
tracking-fennec: + → ---
You need to log in before you can comment on or make changes to this bug.