Closed Bug 912929 Opened 7 years ago Closed 7 years ago

[app manager] UI polish

Categories

(DevTools Graveyard :: WebIDE, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: paul, Assigned: dhenein)

References

Details

Attachments

(4 files, 4 obsolete files)

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.
Initial patch for review, mostly css and image changes to the app-manager UI.
Comment on attachment 803717 [details] [diff] [review]
app_manager_ui-20130912-1017.patch

Ryan, can you take a look at this patch?
Attachment #803717 - Flags: feedback?(jryans)
Remove button bumps into the "HOSTED" label.
This should move it enough to not overlap.
Attachment #803717 - Attachment is obsolete: true
Attachment #803717 - Flags: feedback?(jryans)
Attachment #803909 - Flags: review?(jryans)
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.
Attachment #803717 - Attachment is obsolete: false
(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.  :)
Attached patch devtools app_manager_ui.patch (obsolete) — Splinter Review
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/DarkTheme-Console-SomeToggled@2x.png)
Attachment #803909 - Attachment is obsolete: true
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.
Attachment #803931 - Flags: feedback+
Attachment #803931 - Attachment is obsolete: true
Attachment #803978 - Flags: feedback?(jryans)
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.
Attachment #803978 - Attachment is obsolete: true
Attachment #803978 - Flags: feedback?(jryans)
Attachment #803989 - Flags: review?(paul)
Attachment #803989 - Flags: feedback+
Comment on attachment 803989 [details] [diff] [review]
patch w/ fixed commit message

The rule:

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

is now useless.
Attachment #803989 - Flags: review?(paul) → review+
Attached patch patch to landSplinter Review
I removed the useless rule.
Attachment #803989 - Attachment description: Final patch w/ fixed commit message → patch w/ fixed commit message
Keywords: checkin-needed
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/e31427ba6878
Keywords: checkin-needed
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e31427ba6878
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.