Closed Bug 1212079 Opened 6 years ago Closed 6 years ago

Remove contacts code from the panel

Categories

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

defect

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Iteration:
44.2 - Oct 19
Tracking Status
firefox44 --- fixed

People

(Reporter: standard8, Assigned: crafuse)

References

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 files, 1 obsolete file)

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.
Rank: 12
User Story: (updated)
Assignee: nobody → chris.rafuse
Status: NEW → ASSIGNED
Blocks: 1213984
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)
Showcase removal
Test files removal
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+
User Story: (updated)
Attached image modifiedFilesList.png
to show where further removal may need to happen.
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+
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.
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+
https://hg.mozilla.org/mozilla-central/rev/5bf43b7a4e7b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1216368
Iteration: --- → 44.2 - Oct 19
You need to log in before you can comment on or make changes to this bug.