1212079 - Remove contacts code from the panel

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mark Banner (:standard8)]

After bug 1212074, we should remove the contacts views from the panel. See user story for more details.

Removal of backend LoopContacts will be for a follow-up bug.

Comment 1

[Chris Rafuse :crafuse]

Attachment: Partial remove contacts files list 1/3

removal of jsm and other assets may need assistance on how or what to remove.  See other remove file list attachments on bug page.

Comment 2

[Chris Rafuse :crafuse]

Attachment: Partial remove contacts UI files list 2/3

Showcase removal

Comment 3

[Chris Rafuse :crafuse]

Attachment: Partial remove contacts Test files list 3/3

Test files removal

Comment 4

[Mark Banner (:standard8)]

Removal of jsm & backend code is bug 1213984 (there's more than just the LoopContacts file) - this bug just needs to remove the frontend code.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Looks like you're on a reasonable track.  As Mark mentioned, all the backend/jsm stuff can be omitted.  Also note that the test coverage directories are all auto generated, so they can be as omitted as well.  The graphics sprites, of course, will need surgical excision, but this should be pretty easy.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8673182 [details]
Partial remove contacts files list 1/3

I've just skimmed the file lists in these pngs and left my general comments.  It'll be straightforward to give better feedback/review once you've got something up either as a pull request or a diff.

Comment 7

[Chris Rafuse :crafuse]

Attachment: Removed contact scripts from index

Comment 8

[Chris Rafuse :crafuse]

Attachment: modifiedFilesList.png

Comment 9

[Chris Rafuse :crafuse]

Attachment: contact Files List After Removal Commit

to show where further removal may need to happen.

Comment 10

[Chris Rafuse :crafuse]

Comment on attachment 8674566 [details] [diff] [review]
Removed contact scripts from index

To pass tests I had to remove tests that may need to separated, mainly: mochitest/browser.ini.

Files list
	modified:   browser/components/loop/.eslintignore
	deleted:    browser/components/loop/content/css/contacts.css
	modified:   browser/components/loop/content/css/panel.css
	deleted:    browser/components/loop/content/js/contacts.js
	deleted:    browser/components/loop/content/js/contacts.jsx
	deleted:    browser/components/loop/content/shared/img/empty_contacts.svg
	modified:   browser/components/loop/content/shared/img/icons-14x14.svg
	modified:   browser/components/loop/content/shared/img/icons-16x16.svg
	modified:   browser/components/loop/jar.mn
	deleted:    browser/components/loop/test/desktop-local/contacts_test.js
	modified:   browser/components/loop/test/desktop-local/index.html
	modified:   browser/components/loop/test/karma/karma.coverage.desktop.js
	modified:   browser/components/loop/test/mochitest/browser.ini
	modified:   browser/components/loop/ui/fake-mozLoop.js
	modified:   browser/components/loop/ui/index.html
	modified:   browser/components/loop/ui/ui-showcase.js
	modified:   browser/components/loop/ui/ui-showcase.jsx
	modified:   browser/locales/en-US/chrome/browser/loop/loop.properties

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8674566 [details] [diff] [review]
Removed contact scripts from index

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

Thanks, this looks good.  We can make these changes and then land them.

r=dmose

::: browser/components/loop/content/css/contacts.css
@@ -293,5 @@
> -  -moz-margin-start: 10px;
> -  color: #fff;
> -  -moz-user-select: none;
> -  flex: none;
> -}

This still needs to be removed from ui-showcase.jsx and ui-showcase.css and we should make sure that usage of the icons selector hasn't leaked out to any code that we still use.

::: browser/components/loop/content/css/panel.css
@@ +135,5 @@
>  .no-conversations-message {
>    /* example of vertical aligning a container in an element
>      see: http://zerosixthree.se/vertical-align-anything-with-just-3-lines-of-css/ */
> +  background-repeat: no-repeat;
> +  background-position: top center;

I'd suggest moving these lines to be with the background-image rule.

Comment 12

[Mark Banner (:standard8)]

Err, just jumping in here: On the testing front, we shouldn't be removing browser_mozLoop_pluralStrings.js - that just needs updating to test against strings.

browser_LoopContacts.js I don't understand why that would fail, but since its going away in bug 1213984, I'm not fussed about it being removed here.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Good catch on the mochitest removal, Mark, thanks!  Will fix up before landing.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

New patch will contain cleaned up firefox.js and ui-showcase.jsx as well.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Removed contact scripts from index

This fixes up the explicit review comments and other bits found via grep.  Standard8, after looking at the plural strings stuff more closely, it became clear that the contact strings were the _only_ thing being tested there.  Instead of leaving unused infrastructure in the tree, I've gone ahead and removed it.  If that was a mistake, I apologize, and we can easily resurrect it from version-control.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5bf43b7a4e7b

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5bf43b7a4e7b