Closed Bug 913722 Opened 6 years ago Closed 6 years ago

Last project remains displayed after deleting

Categories

(DevTools :: WebIDE, defect)

defect
Not set

Tracking

(firefox26 verified, firefox27 verified)

VERIFIED FIXED
Firefox 27
Tracking Status
firefox26 --- verified
firefox27 --- verified

People

(Reporter: jryans, Assigned: paul)

References

Details

Attachments

(2 files)

If you delete your last project in the My Apps section, the project details remain displayed on the right-hand side after the project disappears.
Assignee: nobody → paul
Attached patch Patch v1Splinter Review
I have to re-select the item because when we call `remove`, the all list is rebuilt (sic).
Attachment #805969 - Flags: review?(poirot.alex)
Status: NEW → ASSIGNED
Whiteboard: [needed-in-aurora-26]
Comment on attachment 805969 [details] [diff] [review]
Patch v1

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

Looks good, would be fantastic to get a unit test for the template fix ;)

::: browser/devtools/app-manager/content/projects.js
@@ +172,5 @@
>    },
>  
> +  remove: function(location, event) {
> +    if (event) {
> +      event.stopPropagation();

Is it really important to stop the event?
If yes, that's not obvious and a comment would help.

@@ +178,5 @@
> +
> +    let item = document.getElementById(location);
> +
> +    let toSelect = document.querySelector(".project-item.selected");
> +    toSelect = toSelect.id;

toSelect is undefined and it throws when you remove a project when just opening the dashboard (i.e. when no project it selected yet)

::: browser/devtools/app-manager/content/template.js
@@ +340,5 @@
> +        // Nothing to show.
> +        this._unregisterNodes(e.querySelectorAll("[template]"));
> +        e.innerHTML = "";
> +        return;
> +      }

What about adding a test against that in browser/devtools/app-manager/test/test_template.html?
Attachment #805969 - Flags: review?(poirot.alex) → review+
Thanks Alex. I addressed your comments in this patch.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a25028c682ea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Comment on attachment 806655 [details] [diff] [review]
patch to land (with comments addressed)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature (app manager)
User impact if declined: project selection would be a mess
Testing completed (on m-c, etc.): fx-team, m-c
Risk to taking this patch (and alternatives if risky): low 
String or IDL/UUID changes made by this patch: no
Attachment #806655 - Flags: approval-mozilla-aurora?
Attachment #806655 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
I confirm the fix is verified on FF 26b3.
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Windows NT 6.2; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0

Verified as fixed on latest Aurora (buildID: 20131111004004) and latest Nightly (buildID: 20131111030205).
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.