1123525 - [ReadingList] Allow deleting items via the Reading List sidebar

Status: VERIFIED FIXED

(Firefox Graveyard :: Reading List, defect, P1)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We want to be able to delete items from the Reading List sidebar. See bug 1120007 for mockups - each item should have a button on the top right corner.

Unresolved issues:
* Should this should include the possibility of merely archiving the item (ie, marking as read, but keeping it in the list)
* Mockup has the button only appearing on hover. What about touchscreens?

Comment 1

[Michael Maslaney [:mmaslaney] ([e-mail])]

For this V1, let's use the close button on hover. You can also delete an article from Readermode in the Footer. 

V2 will "break more ground", regarding Reading List functionality.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Sorting out strings.... we need tooltip text for the button. Hows:

  "Remove this from your Reading List"

Comment 3

[Michael Maslaney [:mmaslaney] ([e-mail])]

+1

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Mass change of ReadingList bugs, moving to their own component. Filter bugspam on the following quote:

“Reading is to the mind what exercise is to the body.”
― Joseph Addison

Comment 5

[Justin Dolske [:Dolske]]

P1 based on it being the only prominently discoverable way to remove items from RL (the other being the button in the URL bar).

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

Michael, can you drop the delete button icon in this bug?

I found these from your sketch but I'm not sure they are the latest version.
svg: http://cl.ly/1D1d1O0v3E03
png: http://cl.ly/image/1k3W3I1o200h

Comment 7

[Florian Quèze [:florian]]

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #6)
> Michael, can you drop the delete button icon in this bug?
> 
> I found these from your sketch but I'm not sure they are the latest version.
> svg: http://cl.ly/1D1d1O0v3E03
> png: http://cl.ly/image/1k3W3I1o200h

Actually, don't worry, I'm just going to use the same icon as the one used to close the sidebar.

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch

Comment 9

[Florian Quèze [:florian]]

Attachment: Patch

Same patch, with correct context this time.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8579586 [details] [diff] [review]
Patch

Review of attachment 8579586 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/readinglist/sidebar.inc.css
@@ +1,3 @@
> +% This Source Code Form is subject to the terms of the Mozilla Public
> +% License, v. 2.0. If a copy of the MPL was not distributed with this
> +% file, You can obtain one at http://mozilla.org/MPL/2.0/. */

The trailing `*/` can be removed here.

Comment 11

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/6a2887f3dbcd

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a2887f3dbcd

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Florian, we should back this out.

I talked with MattN and he reminded me that we shouldn't be introducing new references to the close-icon. We should instead use the `close-icon` className and then we won't have to duplicate the references.

Comment 14

[Florian Quèze [:florian]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> Florian, we should back this out.
> 
> I talked with MattN and he reminded me that we shouldn't be introducing new
> references to the close-icon. We should instead use the `close-icon`
> className and then we won't have to duplicate the references.

If you are talking about the close-icon class from chrome://global/skin/global.css, it's the first thing I tried, and it's not usable because that global.css file only applies to xul (see the @namespace rule at the top).

Comment 15

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 39.0a1 (2015-03-20) using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS  X 10.9.5, with one minor issue related to the button's tooltip. Filed it as Bug 1145609.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e0c6d66b371

Comment 17

[Andrei Vaida [:avaida]]

Verified fixed on Aurora 38.0a2 (2015-03-29) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.