912929 - [app manager] UI polish

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Paul Rouget [:paul]]

To land the app manager, I had to be aggressive with our CSS code and I had to change the code a lot.

The app manager will land very soon (bug 912447), it would be great to have another round of UI polishing before Firefox 26.

Comment 1

[Darrin Henein [:darrin]]

Attachment: app_manager_ui-20130912-1017.patch

Initial patch for review, mostly css and image changes to the app-manager UI.

Comment 2

[Paul Rouget [:paul]]

Comment on attachment 803717 [details] [diff] [review]
app_manager_ui-20130912-1017.patch

Ryan, can you take a look at this patch?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: App Manager - Remove Overlay.png

Remove button bumps into the "HOSTED" label.

Comment 4

[Darrin Henein [:darrin]]

Attachment: Revised patch with remove button fix.

This should move it enough to not overlap.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: App Manager - Right Border.png

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 803717 [details] [diff] [review]
app_manager_ui-20130912-1017.patch

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

Yay, first patch!  Thanks for the submission.  :D

It looks like you've generated this patch with git format-patch or something like that.  We can only land patches in HG format, even from people working with the Git repo at the moment.  See the patch FAQ[1] for more tips, including tools that help Git users produce an HG formatted patch.

Also, I had some conflicts when applying the patch.  Can you please rebase it against the latest code from fx-team?

If you need help with either of these steps, please ping me on IRC.

Overall, I like the new flat look!  I noticed a few style nits, but hopefully they're not too hard to address.  Take a look at our CSS tips[2] to see more about the CSS style guidelines we use.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2]: https://wiki.mozilla.org/DevTools/CSSTips

::: browser/themes/shared/devtools/app-manager/index.css
@@ +10,4 @@
>  }
>  
>  #tabs {
> +  box-shadow: inset -4px 0 0px rgba(0,0,0,0.3);

Nit: Use 0 without units (no px).  Same for any other 0s.

@@ +15,3 @@
>  }
>  
>  .button {

I like the flatter look!  I noticed there's a right border left over (see attachment #803912 [details]).  Do you think that is still needed now?  I am not sure myself.

@@ +31,4 @@
>    color: #9FA6AD;
>  }
>  
> +.button:first-child{

Nit: Add a space before the brace.

::: browser/themes/shared/devtools/app-manager/projects.css
@@ +96,5 @@
> +.project-item.selected strong {
> +  color: #FFF;
> +}
> +
> +.project-item.selected p, .project-item.selected span {

Nit: Add a new line after the comma with multiple selectors.

@@ +107,5 @@
>    width: 20px;
>    height: 20px;
> +  position: absolute;
> +  right: 10px;
> +  bottom: 10px;

These changes cause the remove button to collide with the "HOSTED", etc. label.  See attachment #803903 [details].  Maybe it's best left how it was?

@@ +266,4 @@
>    color: gray;
>    font-size: 10px;
>    cursor: pointer;
> +  font-family: Courier;

Use monospace instead (and in the rest of these).

::: browser/locales/en-US/chrome/browser/devtools/app-manager.dtd
@@ +46,4 @@
>  <!ENTITY projects.stopApp "Stop">
>  <!ENTITY projects.debugApp "Debug">
>  <!ENTITY projects.hostedManifestPlaceHolder "http://example.com/app/webapp.manifest">
> +<!ENTITY projects.noProject "No projects. Add a new packaged app below (local directory) or a hosted app (link to a manifest file).">

You'll need to update the property ID when changing the text.  For example, change it to "projects.noProject2" and then update the reference as well.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Darrin Henein [:darrin] from comment #4)
> Created attachment 803909 [details] [diff] [review]
> Revised patch with remove button fix.
> 
> This should move it enough to not overlap.

Take a look at rest of my comments, and then I'll take another look.  Thanks for jumping on it so quickly.  I should have hit "Publish" on my comments faster.  :)

Comment 8

[Darrin Henein [:darrin]]

Attachment: devtools app_manager_ui.patch

Fixed all review issues, minus the right-border comment. That is meant to be a sharp drop shadow as seen in some of the new devtools mockups from shorlander (http://people.mozilla.com/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail])

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 803931 [details] [diff] [review]
devtools app_manager_ui.patch

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

Looks good overall!  Thanks for the quick fixes again.

I forgot one thing last time though: you'll need to change the commit message of your patch to match the proper format, like "Bug 123456 - Change this thing to work better by doing something. r=reviewers".  Sometimes you don't know the reviewer name when you first add the patch, so you don't have to include that when you attach (whoever lands the patch will edit it), but it's good to write the rest of the message.  Paul will be the "official" reviewer (since I am not yet approved for reviewing), so you could put "r=paul" if you want to.  Also, don't just use the bug title as your summary.  Attempt a short summary of what changed.

So, attach an updated patch with that change and f? me. I'll double-check it, and if it looks good, I'll r? Paul for final approval.

Comment 10

[Darrin Henein [:darrin]]

Attachment: app_manager_ui.patch w/ proper commit msg

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: patch w/ fixed commit message

Hmm, somehow there is still a "[PATCH]" in the commit message...  Anyway, just re-uploading the patch without that.

Paul, this looks good to me.  Handing off to you for official review.

Comment 12

[Paul Rouget [:paul]]

Comment on attachment 803989 [details] [diff] [review]
patch w/ fixed commit message

The rule:

```
.connected .device-button {
  ...
}
```

is now useless.

Comment 13

[Paul Rouget [:paul]]

Attachment: patch to land

I removed the useless rule.

Comment 14

[Paul Rouget [:paul]]

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

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e31427ba6878