aesqe / firefox-downloads-sidebar

A Firefox web extension that displays a list of your recent downloads in the sidebar
https://addons.mozilla.org/en-US/firefox/addon/downloads-sidebar/
MIT License
17 stars 3 forks source link

Updated visuals to latest Firefox style. #12

Closed josalhor closed 6 years ago

josalhor commented 6 years ago

I focused on the visuals and got what I consider an improvement over the default icons and visuals.

Firefox WebExtensions API is limited on most of the features I wanted to add, so after trying and failing I decided to document that those features can not be implemented.

I also updated Ractive library to the latest sub-version (still 0.9).

What do you think? @aesqe

josalhor commented 6 years ago

@aesqe whenever you can take a look at it, I think it looks better, but as that is something completely subjective I want to hear your thoughts.

aesqe commented 6 years ago

Thank you, @josalhor, and forgive me for the long wait. I appreciate the effort, but I do have just a few questions and suggestions :)

Could you take a look at Mozilla's Photon Icons Library and see if you can use the icons from there? I think all of those needed by the extension should be available in the library. Also, is there a particular reason why the downloads icon is green in your PR? Is it just to differentiate it better from the default downloads?

And lastly, if you have the time and will, do you think you could update the buttons' CSS so it follows the Photon Design guidelines exactly, with all the button states? (https://design.firefox.com/photon/components/buttons.html#default-4, https://design.firefox.com/photon/components/buttons.html#ghost-3).

Thanks!

josalhor commented 6 years ago

Oh yeah, I can absolutely check out those icons, give me a couple of days and I'll take a look at them.

Yes, there is an important reason for the change in the default icon color. The black icon looks really bad on the default dark theme in firefox (that I use) so I figured a green icon is better. Screenshot: https://i.imgur.com/M33is7R.png

I also can update the buttons to follow all the states. I think I already added some of the missing ones, but adding the rest is not a problem. I had it in my mind, but I thought that procedural changes were better, but if you're okay with it I can implement the rest.

An idea I have that could also be implemented: Right now the blue background in every item is fixed and doesn't really add that much information. Would it be better if it had a kind of burst and ease effect on double click (given the delay on opening downloads), so that the blue background has a purpose?

josalhor commented 6 years ago

I've just updated the icons, I plan on changing the animations as soon as I can. Retributions have been updated properly.

Please note 8cb5b5a, I found a bug while doing some testing on the icons, the solution is far from elegant, but it works. Steps to reproduce: Open new download, pause, cancel while paused, the download seems like it can resume.

The Mozilla's Photon Icons Library is far from perfect. There's no play button other than the media one, that I've tested and looks terrible in my honest opinion. I've decided to keep the current one. I've also ignored the block of the js preventing me from downloading the pause icons, because there isn't an updated pause icon.

In general, the update on the icons looks cool.

Please don't mergee yet, as I will properly revise the README when the branch is ready.

josalhor commented 6 years ago

cc/@aesqe I've been working on some important changes that you should check out.

I've been working on:

Tell me what you think about it, because once its merged I'll probably work on #11 .

Edit: Before pulling give me notice so I can update the screenshot of the addon. Or do it yourself :P

josalhor commented 6 years ago

Hey @aesqe , any updates?

aesqe commented 6 years ago

@josalhor Most of the code is good and I look forward to merging it after a few changes are made - thank you for your contribution. 👍

And again, sorry for the long wait.

aesqe commented 6 years ago

@josalhor Sorry, my mistake, Number/0 === Infinity, not the other way around o.O

It's actually the line below: if remainingSeconds is 0, then Infinity is briefly displayed instead of the download speed.

josalhor commented 6 years ago

Under the assumption that if there is less than a second of download we're not downloading anything any more (or by time interval it doesn't matter) I'm returning "0". I can't think of a better return value for that function. I think that covers everything in the review. :)

aesqe commented 6 years ago

Agreed :)

aesqe commented 6 years ago

@josalhor If there's nothing else you'd like to add to this pull request, I'll merge it and prepare another release.

josalhor commented 6 years ago

I believe this pull request is good as it is. I believe #11 could easily be done, but if that's ok with you I would prefer to do a second pull request. I see now you're more active so I don't feel the rush to feed that into this release.

I'm very exited to see this reach the add-ons webpage, I absolutely love this extension. EDIT: CANCEL THE MERGE, I NEED TO UPDATE THE SCREENSHOT

josalhor commented 6 years ago

Nevermind, screenshot updated

aesqe commented 6 years ago

Merged and released on Firefox Extensions, thanks again for the contribution! :)