Closed Bug 1074880 Opened 7 years ago Closed 7 years ago

Remove file icons on file tree view

Categories

(DevTools Graveyard :: WebIDE, defect)

28 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: jfong, Assigned: jfong)

Details

Attachments

(1 file, 1 obsolete file)

We probably don't need images to represent the file types in the file viewer. They take up space and popular editors like Sublime just have the filename only.
+1
Attached patch bug1074880.patch (obsolete) — Splinter Review
I think this is okay, but wanted to make sure I didn't actually blow up something else that might have been using this too.
Attachment #8499669 - Flags: feedback?(jryans)
Comment on attachment 8499669 [details] [diff] [review]
bug1074880.patch

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

This looks good to me.  It doesn't look like your patch actually deletes the PNG, so we should do that assuming Brian agrees below.  (Bugzilla tends to fail at showing binary file changes, so I wasn't completely sure.)

Brian, did we have other consumers of project editor planned that might want these icons?  If we do, we could make this a project editor option, or we could delete them for now, and revive them from source history if we do end up wanting them later.
Attachment #8499669 - Flags: feedback?(jryans)
Attachment #8499669 - Flags: feedback?(bgrinstead)
Attachment #8499669 - Flags: feedback+
Comment on attachment 8499669 [details] [diff] [review]
bug1074880.patch

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

> Brian, did we have other consumers of project editor planned that might want
> these icons?  If we do, we could make this a project editor option, or we
> could delete them for now, and revive them from source history if we do end
> up wanting them later.

Not that I know of.  I don't feel too strongly about the icons either way - I actually kind of like them for things like distinguishing empty folders that don't have arrows by them - but I'm fine with removing them entirely and reviving them if we want them back in the future.  The binary file does seem to be deleted in this patch, so that's already taken care of.

The only change I'd request is that all references to this.icon in tree.js get removed as well.

::: browser/devtools/projecteditor/lib/tree.js
@@ -124,5 @@
>      this.elt.hidden = !visible;
>  
>      this.tree.options.resourceFormatter(this.resource, this.label);
>  
> -    this.icon.className = "file-icon";

this.icon is created and deleted elsewhere in this file - those references should be removed as well
Attachment #8499669 - Flags: feedback?(bgrinstead) → feedback+
Status: NEW → ASSIGNED
Jen, here is a random tip for doing frontend work on the project editor - I usually just open this URL in a tab: chrome://browser/content/devtools/projecteditor-loader.xul.  Then I can use devtools on it so it makes styling changes a lot easier.
Attached patch bug1074880.patchSplinter Review
Attachment #8499669 - Attachment is obsolete: true
Attachment #8500631 - Flags: review?(jryans)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3efeefd11c1a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.