Closed Bug 1057958 Opened 10 years ago Closed 10 years ago

[webide] "reload" button

Categories

(DevTools Graveyard :: WebIDE, defect)

x86_64
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: paul, Assigned: jfong)

Details

Attachments

(2 files, 3 obsolete files)

      No description provided.
We want the play button to have 2 icons: play and reload.
Summary: [webide] → [webide] "reload" button
Darrin, the same way we have play/stop/pause, can we get a "reload" icon? http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webide/themes/icons.png
Flags: needinfo?(dhenein)
Assignee: nobody → shorlander
Flags: needinfo?(dhenein)
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #2)
> Darrin, the same way we have play/stop/pause, can we get a "reload" icon?
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webide/themes/
> icons.png

Do you want me to try and add it to that sprite? If I follow that format it will break all the coordinates.
Flags: needinfo?(paul)
(In reply to Stephen Horlander [:shorlander] from comment #3)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #2)
> > Darrin, the same way we have play/stop/pause, can we get a "reload" icon?
> > http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webide/themes/
> > icons.png
> 
> Do you want me to try and add it to that sprite? If I follow that format it
> will break all the coordinates.

Not a problem. Thank you!
Flags: needinfo?(paul)
Attached image icons.png - 01 (obsolete) —
Added Reload Icon to the sprite sheet.
Flags: needinfo?(paul)
(In reply to Stephen Horlander [:shorlander] from comment #5)
> Created attachment 8494528 [details]
> icons.png - 01
> 
> Added Reload Icon to the sprite sheet.

Perfect! Thank you so much!!!
Flags: needinfo?(paul)
Jen, can you take a look at this?

I should involve:

* Updating the sprite map and coordinates for icons
* Changing the play button to switch to the reload icon once the app is already launched / running
Assignee: shorlander → jfong
Status: NEW → ASSIGNED
Attached patch bug1057958.patch (obsolete) — Splinter Review
Attachment #8495594 - Flags: review?(jryans)
Comment on attachment 8495594 [details] [diff] [review]
bug1057958.patch

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

Thanks for doing this, especially with the sprite sheet.  I know it's annoying to update all the locations... :/

However, I just noticed that the new image seems to have removed the blue phone icon that's used once the runtime in connected.  So, we'll need Shorlander to put that back first.
Attachment #8495594 - Flags: review?(jryans)
Shorlander, please restore the blue phone icon in the new sprite sheet.
Flags: needinfo?(shorlander)
Attached image icons.png - 02
Oops, sorry about that.
Attachment #8494528 - Attachment is obsolete: true
Flags: needinfo?(shorlander)
Attached patch bug1057958.patch (obsolete) — Splinter Review
Attachment #8495594 - Attachment is obsolete: true
Attachment #8496192 - Flags: review?(jryans)
Comment on attachment 8496192 [details] [diff] [review]
bug1057958.patch

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

Great!  Looks good to me.
Attachment #8496192 - Flags: review?(jryans) → review+
Attached patch bug1057958.patchSplinter Review
small css update
Attachment #8496192 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to Jen Fong-Adwent [:ednapiranha] from comment #15)
> Created attachment 8497402 [details] [diff] [review]
> bug1057958.patch
> 
> small css update

With a small change like this after you've already gotten r+, it's good practice to "carry over" the r+ and set it yourself on the new patch.

This way, the sheriffs who land things when you set "checkin-needed" can be sure there was a review at some point.
Attachment #8497402 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/076790f9ebc2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Shouldn't this be uplifted to Firefox 34 beta so that there are no ambiguous messages when one accidentally wants to add the same app twice?
Flags: needinfo?(jryans)
Petruta, I'm not sure if it's too late for that or not.  

Jen, do you want to nominate this for uplift to 34, or maybe 35?
Flags: needinfo?(jfong)
I don't see why not if it's not too late.
Flags: needinfo?(jfong)
As this is mainly a usability improvement and not a regression or bug fix, it seems more appropriate to let it ride the trains.

It landed with 35, so it's already there in Dev. Edition.  I think that's good enough.

If others feel more strongly though, feel free to request beta uplift. :)
Flags: needinfo?(jryans)
Oops. Not sure what I was smoking this morning but yeah it's on 35 already. :)  Good enough!
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.