Remove contacts code from the panel

RESOLVED FIXED in Firefox 44

Status

Hello (Loop)
Client
P2
normal
Rank:
12
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: crafuse)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [web sharing])

User Story

Acceptance criteria:

- All contact related views removed.
- All associated css removed.
- All redundant strings removed.
- Views removed from ui-showcase.
- All related tests removed.
- Any redundant prefs removed from firefox.js

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Rank: 12
(Reporter)

Updated

2 years ago
User Story: (updated)
(Assignee)

Updated

2 years ago
Assignee: nobody → chris.rafuse
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
Blocks: 1213984
(Assignee)

Comment 1

2 years ago
Created attachment 8673182 [details]
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.
Attachment #8673182 - Flags: feedback?(dmose)
(Assignee)

Comment 2

2 years ago
Created attachment 8673183 [details]
Partial remove contacts UI files list 2/3

Showcase removal
(Assignee)

Comment 3

2 years ago
Created attachment 8673184 [details]
Partial remove contacts Test files list 3/3

Test files removal
(Reporter)

Comment 4

2 years ago
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.
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 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.
Attachment #8673182 - Flags: feedback?(dmose) → feedback+
(Assignee)

Updated

2 years ago
User Story: (updated)
(Assignee)

Comment 7

2 years ago
Created attachment 8674566 [details] [diff] [review]
Removed contact scripts from index
(Assignee)

Comment 8

2 years ago
Created attachment 8674567 [details]
modifiedFilesList.png
(Assignee)

Comment 9

2 years ago
Created attachment 8674571 [details]
contact Files List After Removal Commit

to show where further removal may need to happen.
(Assignee)

Comment 10

2 years ago
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
Attachment #8674566 - Flags: ui-review?(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.
Attachment #8674566 - Flags: ui-review?(dmose) → ui-review+
(Reporter)

Comment 12

2 years ago
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.
Good catch on the mochitest removal, Mark, thanks!  Will fix up before landing.
New patch will contain cleaned up firefox.js and ui-showcase.jsx as well.
Created attachment 8675029 [details] [diff] [review]
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.
Attachment #8675029 - Flags: review+

Updated

2 years ago
Attachment #8674566 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5bf43b7a4e7b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

2 years ago
Depends on: 1216368

Updated

2 years ago
Iteration: --- → 44.2 - Oct 19
You need to log in before you can comment on or make changes to this bug.